diff --git a/core/local/index.js b/core/local/index.js index f29478f0e..5f5a184ff 100644 --- a/core/local/index.js +++ b/core/local/index.js @@ -15,6 +15,7 @@ const bluebird = require('bluebird') const { TMP_DIR_NAME } = require('./constants') const { NOTE_MIME_TYPE } = require('../remote/constants') +const { isRetryableNetworkError } = require('../remote/errors') const stater = require('./stater') const metadata = require('../metadata') const { hideOnWindows } = require('../utils/fs') @@ -251,46 +252,49 @@ class Local /*:: implements Reader, Writer */ { } }, - async existingFilePath => { - return new Promise((resolve, reject) => { - fse.ensureDir(this.tmpPath, async () => { - hideOnWindows(this.tmpPath) - if (existingFilePath) { - log.info( - { path: filePath }, - `Recopy ${existingFilePath} -> ${filePath}` - ) - this.events.emit('transfer-copy', doc) - fse.copy(existingFilePath, tmpFile, err => { - if (err) { - reject(err) - } else { - resolve() - } - }) - } else { - try { - const reader = await this.other.createReadStreamAsync(doc) - const source = onProgress - ? streamUtils.withProgress(reader, onProgress) - : reader - - const destination = fse.createWriteStream(tmpFile) - - stream.pipeline(source, destination, err => { + async.retryable( + { times: 5, interval: 2000, errorFilter: isRetryableNetworkError }, + async existingFilePath => { + return new Promise((resolve, reject) => { + fse.ensureDir(this.tmpPath, async () => { + hideOnWindows(this.tmpPath) + if (existingFilePath) { + log.info( + { path: filePath }, + `Recopy ${existingFilePath} -> ${filePath}` + ) + this.events.emit('transfer-copy', doc) + fse.copy(existingFilePath, tmpFile, err => { if (err) { reject(err) } else { resolve() } }) - } catch (err) { - reject(err) + } else { + try { + const reader = await this.other.createReadStreamAsync(doc) + const source = onProgress + ? streamUtils.withProgress(reader, onProgress) + : reader + + const destination = fse.createWriteStream(tmpFile) + + stream.pipeline(source, destination, err => { + if (err) { + reject(err) + } else { + resolve() + } + }) + } catch (err) { + reject(err) + } } - } + }) }) - }) - }, + } + ), async () => { if (doc.md5sum != null) { diff --git a/core/remote/errors.js b/core/remote/errors.js index b7bf9c6e4..6d155ff19 100644 --- a/core/remote/errors.js +++ b/core/remote/errors.js @@ -188,7 +188,7 @@ const wrapError = ( err /*: FetchError | Error */, doc /*: ?SavedMetadata */ ) /*: RemoteError */ => { - if (err.name === 'FetchError') { + if (isNetworkError(err)) { // $FlowFixMe FetchErrors missing status will fallback to the default case const { status } = err @@ -379,6 +379,22 @@ function detail(err /*: FetchError */) /*: ?string */ { return detail } +function isNetworkError(err /*: Error */) { + return ( + err.name === 'FetchError' || + (typeof err.message === 'string' && err.message.includes('net::')) + ) +} + +function isRetryableNetworkError(err /*: Error */) { + return ( + typeof err.message === 'string' && + err.message.includes('net::') && + !err.message.includes('net::ERR_INTERNET_DISCONNECTED') && + !err.message.includes('net::ERR_PROXY_CONNECTION_FAILED') + ) +} + module.exports = { CozyDocumentMissingError, DirectoryNotFound, @@ -405,5 +421,7 @@ module.exports = { UNKNOWN_REMOTE_ERROR_CODE, UNREACHABLE_COZY_CODE, USER_ACTION_REQUIRED_CODE, + isNetworkError, + isRetryableNetworkError, wrapError } diff --git a/core/remote/index.js b/core/remote/index.js index 057596c39..6751120b4 100644 --- a/core/remote/index.js +++ b/core/remote/index.js @@ -7,6 +7,7 @@ const autoBind = require('auto-bind') const Promise = require('bluebird') const path = require('path') +const async = require('async') const logger = require('../utils/logger') const measureTime = require('../utils/perfs') @@ -14,7 +15,11 @@ const pathUtils = require('../utils/path') const metadata = require('../metadata') const { ROOT_DIR_ID, DIR_TYPE } = require('./constants') const { RemoteCozy } = require('./cozy') -const { DirectoryNotFound, ExcludedDirError } = require('./errors') +const { + DirectoryNotFound, + ExcludedDirError, + isRetryableNetworkError +} = require('./errors') const { RemoteWarningPoller } = require('./warning_poller') const { RemoteWatcher } = require('./watcher') const timestamp = require('../utils/timestamp') @@ -182,32 +187,37 @@ class Remote /*:: implements Reader, Writer */ { const [parentPath, name] = dirAndName(path) const parent = await this.findDirectoryByPath(parentPath) - let stream - try { - stream = await this.other.createReadStreamAsync(doc) - } catch (err) { - if (err.code === 'ENOENT') { - log.warn({ path }, 'Local file does not exist anymore.') - // FIXME: with this deletion marker, the record will be erased from - // PouchDB while the remote document will remain. - doc.trashed = true - return doc - } - throw err - } - - const source = onProgress - ? streamUtils.withProgress(stream, onProgress) - : stream + await async.retry( + { times: 5, interval: 2000, errorFilter: isRetryableNetworkError }, + async () => { + let stream + try { + stream = await this.other.createReadStreamAsync(doc) + } catch (err) { + if (err.code === 'ENOENT') { + log.warn({ path }, 'Local file does not exist anymore.') + // FIXME: with this deletion marker, the record will be erased from + // PouchDB while the remote document will remain. + doc.trashed = true + return doc + } + throw err + } - const created = await this.remoteCozy.createFile(source, { - ...newDocumentAttributes(name, parent._id, doc.updated_at), - checksum: doc.md5sum, - executable: doc.executable || false, - contentLength: doc.size, - contentType: doc.mime - }) - metadata.updateRemote(doc, created) + const source = onProgress + ? streamUtils.withProgress(stream, onProgress) + : stream + + const created = await this.remoteCozy.createFile(source, { + ...newDocumentAttributes(name, parent._id, doc.updated_at), + checksum: doc.md5sum, + executable: doc.executable || false, + contentLength: doc.size, + contentType: doc.mime + }) + metadata.updateRemote(doc, created) + } + ) stopMeasure() } @@ -219,46 +229,51 @@ class Remote /*:: implements Reader, Writer */ { const { path } = doc log.info({ path }, 'Uploading new file version...') - let stream - try { - stream = await this.other.createReadStreamAsync(doc) - } catch (err) { - if (err.code === 'ENOENT') { - log.warn({ path }, 'Local file does not exist anymore.') - // FIXME: with this deletion marker, the record will be erased from - // PouchDB while the remote document will remain. - doc.trashed = true - return doc - } - throw err - } + await async.retry( + { times: 5, interval: 2000, errorFilter: isRetryableNetworkError }, + async () => { + let stream + try { + stream = await this.other.createReadStreamAsync(doc) + } catch (err) { + if (err.code === 'ENOENT') { + log.warn({ path }, 'Local file does not exist anymore.') + // FIXME: with this deletion marker, the record will be erased from + // PouchDB while the remote document will remain. + doc.trashed = true + return doc + } + throw err + } - // Object.assign gives us the opportunity to enforce required options with - // Flow while they're only optional in the Metadata type. For example, - // `md5sum` and `mime` are optional in Metadata because they only apply to - // files. But we're sure we have files at this point and that they do have - // those attributes. - const options = Object.assign( - {}, - { - checksum: doc.md5sum, - executable: doc.executable || false, - contentLength: doc.size, - contentType: doc.mime, - updatedAt: mostRecentUpdatedAt(doc), - ifMatch: doc.remote._rev + // Object.assign gives us the opportunity to enforce required options with + // Flow while they're only optional in the Metadata type. For example, + // `md5sum` and `mime` are optional in Metadata because they only apply to + // files. But we're sure we have files at this point and that they do have + // those attributes. + const options = Object.assign( + {}, + { + checksum: doc.md5sum, + executable: doc.executable || false, + contentLength: doc.size, + contentType: doc.mime, + updatedAt: mostRecentUpdatedAt(doc), + ifMatch: doc.remote._rev + } + ) + const source = onProgress + ? streamUtils.withProgress(stream, onProgress) + : stream + + const updated = await this.remoteCozy.updateFileById( + doc.remote._id, + source, + options + ) + metadata.updateRemote(doc, updated) } ) - const source = onProgress - ? streamUtils.withProgress(stream, onProgress) - : stream - - const updated = await this.remoteCozy.updateFileById( - doc.remote._id, - source, - options - ) - metadata.updateRemote(doc, updated) } async updateFileMetadataAsync(doc /*: SavedMetadata */) /*: Promise */ { diff --git a/core/remote/watcher/index.js b/core/remote/watcher/index.js index c01e7f6fe..49a06df47 100644 --- a/core/remote/watcher/index.js +++ b/core/remote/watcher/index.js @@ -169,6 +169,11 @@ class RemoteWatcher { async watch() /*: Promise */ { const release = await this.pouch.lock(this) try { + if (!this.running) { + log.debug('Watcher stopped: skipping remote watch') + return + } + this.events.emit('buffering-start') const seq = await this.pouch.getRemoteSeq() diff --git a/core/sync/errors.js b/core/sync/errors.js index 4723fd41c..f4e9dbbf2 100644 --- a/core/sync/errors.js +++ b/core/sync/errors.js @@ -295,10 +295,10 @@ const wrapError = ( return new SyncError({ sideName, err, code: INCOMPATIBLE_DOC_CODE, doc }) } else if (err instanceof remoteErrors.ExcludedDirError) { return new SyncError({ sideName, err, code: EXCLUDED_DIR_CODE, doc }) - } else if (sideName === 'remote' || err.name === 'FetchError') { + } else if (remoteErrors.isNetworkError(err)) { // FetchErrors can be raised from the LocalWriter when failing to download a - // file for example. In this case the sideName will be "local" but the error - // name will still be "FetchError". + // file for example. In this case the error name won't be "FetchError" but + // its message will still contain `net::`. // If err is a RemoteError, its code will be reused. return new SyncError({ sideName, diff --git a/core/utils/lifecycle.js b/core/utils/lifecycle.js index 6c7f4caf9..e88f3d730 100644 --- a/core/utils/lifecycle.js +++ b/core/utils/lifecycle.js @@ -14,14 +14,14 @@ type State = 'done-stop' | 'will-start' | 'done-start' | 'will-stop' class LifeCycle extends EventEmitter { /*:: currentState: State - blockedFor: Set + blockedFor: ?string log: Logger */ constructor(logger /*: Logger */) { super() this.currentState = 'done-stop' - this.blockedFor = new Set() + this.blockedFor = null this.log = logger } @@ -89,18 +89,21 @@ class LifeCycle extends EventEmitter { } blockFor(reason /*: string */) { - this.blockedFor.add(reason) + this.log.debug(`blocking for ${reason}`) + this.blockedFor = reason } unblockFor(reason /*: string */) { - if (reason === 'all') this.blockedFor.clear() - else this.blockedFor.delete(reason) - if (this.blockedFor.size === 0) this.emit('ready') + this.log.debug(`unblocking for ${reason}`) + if (reason === 'all' || reason === this.blockedFor) { + this.blockedFor = null + this.emit('ready') + } } async ready() /*: Promise */ { return new Promise(resolve => { - if (this.blockedFor.size === 0) resolve() + if (this.blockedFor == null) resolve() else this.once('ready', resolve) }) } diff --git a/test/support/helpers/remote.js b/test/support/helpers/remote.js index da0f5f7bb..71db32678 100644 --- a/test/support/helpers/remote.js +++ b/test/support/helpers/remote.js @@ -51,7 +51,9 @@ class RemoteTestHelpers { } async pullChanges() { + this.side.watcher.running = true await this.side.watcher.watch() + this.side.watcher.running = false } async createDirectory( diff --git a/test/unit/remote/errors.js b/test/unit/remote/errors.js new file mode 100644 index 000000000..119de81f5 --- /dev/null +++ b/test/unit/remote/errors.js @@ -0,0 +1,18 @@ +/* eslint-env mocha */ +/* @flow */ + +const should = require('should') + +const remoteErrors = require('../../../core/remote/errors') + +describe('Remote.wrapError', () => { + it('returns an UnreachableCozy error for net::ERR_NETWORKD_CHANGED errors', () => { + const netErr = new Error( + 'Failed request, reason: net::ERR_NETWORKD_CHANGED' + ) + should(remoteErrors.wrapError(netErr)).have.property( + 'code', + remoteErrors.UNREACHABLE_COZY_CODE + ) + }) +}) diff --git a/test/unit/remote/watcher.js b/test/unit/remote/watcher.js index 50e81d559..0d61ffc01 100644 --- a/test/unit/remote/watcher.js +++ b/test/unit/remote/watcher.js @@ -390,6 +390,7 @@ describe('RemoteWatcher', function () { sinon.spy(this.events, 'emit') this.pouch.getRemoteSeq.resolves(lastLocalSeq) + this.watcher.running = true this.watcher.processRemoteChanges.resolves([]) this.remoteCozy.changes.resolves(changes) }) @@ -398,6 +399,7 @@ describe('RemoteWatcher', function () { this.events.emit.restore() this.remoteCozy.changes.restore() this.watcher.processRemoteChanges.restore() + this.watcher.running = false this.pouch.setRemoteSeq.restore() this.pouch.getRemoteSeq.restore() }) @@ -543,6 +545,34 @@ describe('RemoteWatcher', function () { }) }) }) + + context('when watcher is not running', () => { + beforeEach(function () { + this.watcher.running = false + }) + + afterEach(function () { + this.watcher.running = true + }) + + it('returns without fetching changes', async function () { + await this.watcher.watch() + + should(this.remoteCozy.changes).not.have.been.called() + }) + + it('still tries to get hold of the PouchDB lock', async function () { + sinon.spy(this.pouch, 'lock') + + try { + await this.watcher.watch() + + should(this.pouch.lock).have.been.calledOnce() + } finally { + this.pouch.lock.restore() + } + }) + }) }) const validMetadata = ( diff --git a/test/unit/sync/errors.js b/test/unit/sync/errors.js new file mode 100644 index 000000000..25048a462 --- /dev/null +++ b/test/unit/sync/errors.js @@ -0,0 +1,19 @@ +/* eslint-env mocha */ +/* @flow */ + +const should = require('should') + +const remoteErrors = require('../../../core/remote/errors') +const syncErrors = require('../../../core/sync/errors') + +describe('Sync.wrapError', () => { + it('returns an UnreachableCozy error for net::ERR_NETWORKD_CHANGED errors', () => { + const netErr = new Error( + 'Failed request, reason: net::ERR_NETWORKD_CHANGED' + ) + should(syncErrors.wrapError(netErr, 'local')).have.property( + 'code', + remoteErrors.UNREACHABLE_COZY_CODE + ) + }) +}) diff --git a/test/unit/sync/index.js b/test/unit/sync/index.js index 2facdde6f..84df6553f 100644 --- a/test/unit/sync/index.js +++ b/test/unit/sync/index.js @@ -910,6 +910,40 @@ describe('Sync', function () { this.events.emit.restore() }) + context('when Sync is already blocked for a reason', () => { + const unknownSyncError = syncErrors.wrapError( + new Error('what is going on?'), + 'local' + ) + const unreachableSyncError = syncErrors.wrapError( + new FetchError( + { type: 'system', code: 'ENOTFOUND', errno: 'ENOTFOUND' }, + 'request to ... failed, reason: net::ERR_NAME_NOT_RESOLVED' + ), + 'remote' + ) + beforeEach(function () { + this.sync.blockSyncFor({ + err: unknownSyncError + }) + should(this.sync.lifecycle.blockedFor).equal( + syncErrors.UNKNOWN_SYNC_ERROR_CODE + ) + }) + + it('replaces the old reason with the new one', async function () { + this.sync.blockSyncFor({ + err: unreachableSyncError + }) + should(this.sync.lifecycle.blockedFor).equal( + remoteErrors.UNREACHABLE_COZY_CODE + ) + + this.sync.lifecycle.unblockFor(remoteErrors.UNREACHABLE_COZY_CODE) + should(this.sync.lifecycle.blockedFor).be.null() + }) + }) + context('when Cozy is unreachable', () => { const unreachableSyncError = syncErrors.wrapError( new FetchError(