From fe43e21484f6356e0709b399d03e8796189d4f61 Mon Sep 17 00:00:00 2001 From: Jonas Hermsmeier Date: Fri, 6 Apr 2018 16:01:23 +0200 Subject: [PATCH] fix(writer): Fix erronous event handling in write pipeline This fixes the use and handling of events in the write pipeline, such that the pipeline would not be prematurely stalled or terminated. Also, a new `fail` event is introduced, to signal non-fatal errors. Change-Type: patch --- lib/cli/etcher.js | 13 +----- lib/gui/app/modules/image-writer.js | 4 ++ lib/gui/app/modules/progress-status.js | 2 + lib/gui/modules/child-writer.js | 22 +++++++++ lib/sdk/writer/index.js | 62 ++++++++++++++++++-------- 5 files changed, 73 insertions(+), 30 deletions(-) diff --git a/lib/cli/etcher.js b/lib/cli/etcher.js index 2acf45a6..c9df30aa 100644 --- a/lib/cli/etcher.js +++ b/lib/cli/etcher.js @@ -102,17 +102,6 @@ permissions.isElevated().then((elevated) => { checksumAlgorithms: options.check ? [ 'sha512' ] : [] }) - /** - * @summary Error handler - * @param {Error} error - error - * @private - * @example - * writer.on('error', onError) - */ - const onError = function (error) { - console.error(error) - } - /** * @summary Finish handler * @private @@ -124,7 +113,7 @@ permissions.isElevated().then((elevated) => { } writer.on('progress', onProgress) - writer.on('error', onError) + writer.on('error', reject) writer.on('finish', onFinish) // NOTE: Drive can be (String|Array) diff --git a/lib/gui/app/modules/image-writer.js b/lib/gui/app/modules/image-writer.js index b1005532..0d765cdf 100644 --- a/lib/gui/app/modules/image-writer.js +++ b/lib/gui/app/modules/image-writer.js @@ -126,6 +126,10 @@ exports.performWrite = (image, drives, onProgress) => { console.log(message) }) + ipc.server.on('fail', (error) => { + console.log('Fail:', error) + }) + const flashResults = {} ipc.server.on('done', (results) => { _.merge(flashResults, results) diff --git a/lib/gui/app/modules/progress-status.js b/lib/gui/app/modules/progress-status.js index beac4c5f..ac87ef48 100644 --- a/lib/gui/app/modules/progress-status.js +++ b/lib/gui/app/modules/progress-status.js @@ -61,6 +61,8 @@ exports.fromFlashState = (state) => { return `${state.percentage}% Flashing` } else if (isValidating) { return `${state.percentage}% Validating` + } else if (!isFlashing && !isValidating) { + return 'Failed' } throw new Error(`Invalid state: ${JSON.stringify(state)}`) diff --git a/lib/gui/modules/child-writer.js b/lib/gui/modules/child-writer.js index 30be4fd6..42d9fa6c 100644 --- a/lib/gui/modules/child-writer.js +++ b/lib/gui/modules/child-writer.js @@ -154,6 +154,27 @@ ipc.connectTo(IPC_SERVER_ID, () => { ipc.of[IPC_SERVER_ID].emit('error', error) } + /** + * @summary Failure handler (non-fatal errors) + * @param {Object} event - event data (error & device) + * @example + * writer.on('fail', onFail) + */ + const onFail = (event) => { + ipc.of[IPC_SERVER_ID].emit('fail', { + device: event.device, + error: { + name: event.error.name, + message: event.error.message, + code: event.error.code, + syscall: event.error.syscall, + errno: event.error.errno, + stack: event.error.stack, + stdout: event.error.stdout + } + }) + } + const writer = new ImageWriter({ verify: options.validateWriteOnSuccess, unmountOnSuccess: options.unmountOnSuccess, @@ -161,6 +182,7 @@ ipc.connectTo(IPC_SERVER_ID, () => { }) writer.on('error', onError) + writer.on('fail', onFail) writer.on('progress', onProgress) writer.on('finish', onFinish) diff --git a/lib/sdk/writer/index.js b/lib/sdk/writer/index.js index 1c72623c..4da69218 100644 --- a/lib/sdk/writer/index.js +++ b/lib/sdk/writer/index.js @@ -217,6 +217,7 @@ class ImageWriter extends EventEmitter { mountutils.unmountDisk(destination.device.device, (error) => { debug('state:unmount', destination.device.device, error ? 'NOT OK' : 'OK') + destination.error = error callback.call(this, error) }) } @@ -241,6 +242,7 @@ class ImageWriter extends EventEmitter { diskpart.clean(destination.device.device).asCallback((error) => { debug('state:clean', destination.device.device, error ? 'NOT OK' : 'OK') + destination.error = error callback.call(this, error) }) } @@ -286,12 +288,13 @@ class ImageWriter extends EventEmitter { fs.open(destination.device.raw, flags, (error, fd) => { debug('state:destination-open', destination.device.raw, error ? 'NOT OK' : 'OK') destination.fd = fd + destination.error = error callback.call(this, error) }) } /** - * @summary Check a destinstation against the drive constraints + * @summary Check a destination against the drive constraints * @param {Object} destination - destination object * @param {Function} callback - callback(error) * @example @@ -300,16 +303,14 @@ class ImageWriter extends EventEmitter { * }) */ checkDriveConstraints (destination, callback) { - let error = null - if (!constraints.isDriveLargeEnough(destination.device, this.source)) { - error = errors.createUserError({ + destination.error = errors.createUserError({ title: 'The image you selected is too big for this drive', description: 'Please connect a bigger drive and try again' }) } - callback.call(this, error) + callback.call(this, destination.error) } /** @@ -356,9 +357,11 @@ class ImageWriter extends EventEmitter { (done) => { this.unmountDevice(destination, done) }, (done) => { this.removePartitionTable(destination, done) }, (done) => { this.openDestination(destination, done) } - ], (preparationError) => { - destination.error = preparationError - next(preparationError) + ], () => { + if (destination.error) { + this.emit('fail', { device: destination.device.device, error: destination.error }) + } + next(destination.error, destination) }) } }) @@ -367,7 +370,11 @@ class ImageWriter extends EventEmitter { runParallel(tasks, (resultErrors, results) => { // We can start (theoretically) flashing now... debug('write:prep:done', resultErrors) - this._write() + if (_.every(resultErrors, _.identity)) { + this.emit('error', resultErrors[0]) + } else { + this._write() + } }) }) }) @@ -433,23 +440,28 @@ class ImageWriter extends EventEmitter { this.emit('error', error) }) - this.pipeline.on('finish', (destination) => { + this.pipeline.on('complete', (destination) => { this.bytesRead = this.source.bytesRead let finishedCount = 0 + let errorCount = 0 this.destinations.forEach((dest) => { finishedCount += dest.finished ? 1 : 0 + errorCount += dest.error ? 1 : 0 }) debug('write:finish', finishedCount, '/', this.destinations.size) - if (destination) { + if (_.has(destination, [ 'stream' ])) { this.bytesWritten += destination.stream.bytesWritten } if (finishedCount === this.destinations.size) { - if (this.verifyChecksums) { + if (errorCount === this.destinations.size) { + this.emit('error', destination.error) + this._finish() + } else if (this.verifyChecksums) { debug('write:verify') this.verify() } else { @@ -469,8 +481,20 @@ class ImageWriter extends EventEmitter { * imageWriter.verify() */ verify () { + let bytesWritten = 0 + + // NOTE: We can't re-use `this.bytesWritten` here, as that will + // included bytes of streams that may have errored part way through + this.destinations.forEach((destination) => { + // Don't count errored destinations + if (destination.error || !destination.stream) { + return + } + bytesWritten += destination.stream.bytesWritten + }) + const progressStream = new ProgressStream({ - length: this.bytesWritten, + length: bytesWritten, time: 500 }) @@ -500,7 +524,7 @@ class ImageWriter extends EventEmitter { const error = new Error(`Verification failed: ${JSON.stringify(this.checksum)} != ${JSON.stringify(checksum)}`) error.code = 'EVALIDATION' destination.error = error - this.emit('error', error) + this.emit('fail', { device: destination.device.device, error }) } }) @@ -681,6 +705,7 @@ class ImageWriter extends EventEmitter { this.destinations.forEach((destination) => { if (destination.error) { debug('pipeline:skip', destination.device.device) + destination.finished = true return } @@ -689,21 +714,22 @@ class ImageWriter extends EventEmitter { autoClose: false }) - destination.stream.once('finish', () => { + destination.stream.on('finish', () => { debug('finish:unpipe', destination.device.device) destination.finished = true - pipeline.emit('finish', destination) + pipeline.emit('complete', destination) pipeline.unpipe(destination.stream) }) - destination.stream.once('error', (error) => { + destination.stream.on('error', (error) => { debug('error:unpipe', destination.device.device) destination.error = error destination.finished = true pipeline.unpipe(destination.stream) + this.emit('fail', { device: destination.device.device, error }) + pipeline.emit('complete', destination) }) - pipeline.bind(destination.stream, 'error') pipeline.pipe(destination.stream) })