From 3f4aea20e58800987cfe029d672f42491a5ef5be Mon Sep 17 00:00:00 2001 From: Stefan Penner Date: Mon, 11 Dec 2017 20:12:46 -0800 Subject: [PATCH] WIP: cancellation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ``` builder.cancel().then(() => /* the build was cancelled */); ``` - [x] a fulfilled `cancel` guarantees to leave the build in a resumable state - [x] a rejected `cancel` suggests a unrecoverable build - [x] `builder.cancel()` will currently wait for the current step to an, before cancelling the build. This is to ensure the build is resumable, and can be lifted in the future with more work. - [x] a `builder.buid()` is not allowed if the `builder` currently has a pending build or cancellation - [x] `builder.cancel` is idempotent - [x] events (‘nodeStart’ ‘nodeEnd’) remain balanced in-light or cancellation - [x] CancellationErrors are `isSilent` and `isCancellation` to play nicely with consumers like ember-cli - [x] better error messages TODO: - [ ] mid-build change events should cancel the current build and queue up behind the existing cancel before kicking off another build. --- lib/builder.js | 36 +++++- lib/cancellation-request.js | 37 +++++++ lib/errors/build.js | 3 + lib/errors/builder.js | 5 + lib/errors/cancelation.js | 23 ++++ test/builder_test.js | 177 ++++++++++++++++++++++++++++++ test/cancellation-request-test.js | 52 +++++++++ test/plugins.js | 49 +++++++-- 8 files changed, 369 insertions(+), 13 deletions(-) create mode 100644 lib/cancellation-request.js create mode 100644 lib/errors/cancelation.js create mode 100644 test/cancellation-request-test.js diff --git a/lib/builder.js b/lib/builder.js index 3a326009..3361caab 100644 --- a/lib/builder.js +++ b/lib/builder.js @@ -15,6 +15,7 @@ const SourceNodeWrapper = require('./wrappers/source-node'); const BuilderError = require('./errors/builder'); const NodeSetupError = require('./errors/node-setup'); const BuildError = require('./errors/build'); +const CancelationRequest = require('./cancellation-request'); // Clean up left-over temporary directories on uncaught exception. tmp.setGracefulCleanup(); @@ -61,6 +62,7 @@ module.exports = class Builder { this.checkInputPathsExist(); this.setupTmpDirs(); + this._cancelationRequest = undefined; // Now that temporary directories are set up, we need to run the rest of the // constructor in a try/catch block to clean them up if necessary. @@ -81,8 +83,16 @@ module.exports = class Builder { // This method will never throw, and it will never be rejected with anything // other than a BuildError. build() { + if (this._cancelationRequest) { + return RSVP.Promise.reject( + new BuilderError('Cannot start a build if one is already running') + ); + } + + let promise = RSVP.Promise.resolve(); + this.buildId++; - let promise = RSVP.resolve(); + this.nodeWrappers.forEach(nw => { // We use `.forEach` instead of `for` to close nested functions over `nw` @@ -93,16 +103,36 @@ module.exports = class Builder { // We use a nested .then/.catch so that the .catch can only catch errors // from this node, but not from previous nodes. return RSVP.resolve() + .then(() => this._cancelationRequest.throwIfRequested()) .then(() => this.trigger('beginNode', nw)) .then(() => nw.build()) - .finally(() => this.trigger('endNode', nw)) + .finally(() => { + if (this._cancelationRequest.isCancelled) { + return; + } + this.trigger('endNode', nw); + }) + .then(() => this._cancelationRequest.throwIfRequested()) .catch(err => { throw new BuildError(err, nw); }); }); }); - return promise; + this._cancelationRequest = new CancelationRequest(promise); + + return promise.finally(() => { + this._cancelationRequest = null; + }); + } + + cancel() { + if (!this._cancelationRequest) { + // No current build, so no cancellation + return RSVP.Promise.resolve(); + } + + return this._cancelationRequest.cancel(); } // Destructor-like method. Cleanup is synchronous at the moment, but in the diff --git a/lib/cancellation-request.js b/lib/cancellation-request.js new file mode 100644 index 00000000..261c31e3 --- /dev/null +++ b/lib/cancellation-request.js @@ -0,0 +1,37 @@ +'use strict'; + +const CancelationError = require('./errors/cancelation'); +const BuilderError = require('./errors/builder'); + +module.exports = class CancelationRequest { + constructor(pendingWork) { + this._isCancelled = false; + this._pendingWork = pendingWork; // all + } + + get isCancelled() { + return this._isCancelled; + } + + throwIfRequested() { + if (this._isCancelled) { + throw new CancelationError('BUILD CANCELLED'); + } + } + + cancel() { + // if we ever expose the cancellation to plugins, we should not expose + // cancel(), rather it should be expose similarity to the Promise + // executor + this._isCancelled = true; + + return this._pendingWork.catch(e => { + // a rejection with a cancel cancel promise + if (BuilderError.isBuilderError(e)) { + return; + } + + throw e; + }); + } +}; diff --git a/lib/errors/build.js b/lib/errors/build.js index 9c8805d4..495ad3e6 100644 --- a/lib/errors/build.js +++ b/lib/errors/build.js @@ -44,7 +44,10 @@ module.exports = class BuildError extends BuilderError { instantiationStack; super(message); + // consider for x in y this.stack = originalError.stack; + this.isSilent = originalError.isSilent; + this.isCancellation = originalError.isCancellation; // This error API can change between minor Broccoli version bumps this.broccoliPayload = { diff --git a/lib/errors/builder.js b/lib/errors/builder.js index 0914f465..6b46cfb0 100644 --- a/lib/errors/builder.js +++ b/lib/errors/builder.js @@ -2,6 +2,10 @@ // Base class for builder errors module.exports = class BuilderError extends Error { + static isBuilderError(error) { + return error !== null && typeof error === 'object' && error.isBuilderError; + } + constructor(a, b, c) { // Subclassing Error in ES5 is non-trivial because reasons, so we need this // extra constructor logic from http://stackoverflow.com/a/17891099/525872. @@ -12,5 +16,6 @@ module.exports = class BuilderError extends Error { temp.name = this.name = this.constructor.name; this.stack = temp.stack; this.message = temp.message; + this.isBuilderError = true; } }; diff --git a/lib/errors/cancelation.js b/lib/errors/cancelation.js new file mode 100644 index 00000000..ce0a8cd5 --- /dev/null +++ b/lib/errors/cancelation.js @@ -0,0 +1,23 @@ +'use strict'; + +// Base class for builder errors +module.exports = class Cancellation extends Error { + static isCancellation(e) { + return typeof e === 'object' && e !== null && e.isCancellation == true; + } + + constructor(a, b, c) { + // Subclassing Error in ES5 is non-trivial because reasons, so we need this + // extra constructor logic from http://stackoverflow.com/a/17891099/525872. + // Note that ES5 subclasses of BuilderError don't in turn need any special + // code. + let temp = super(a, b, c); + // Need to assign temp.name for correct error class in .stack and .message + temp.name = this.name = this.constructor.name; + this.stack = temp.stack; + this.message = temp.message; + + this.isCancellation = true; + this.isSilent = true; + } +}; diff --git a/test/builder_test.js b/test/builder_test.js index 9acabec2..91861e85 100644 --- a/test/builder_test.js +++ b/test/builder_test.js @@ -29,6 +29,9 @@ RSVP.on('error', error => { throw error; }); +function wait(time) { + return new RSVP.Promise(resolve => setTimeout(resolve, time || 0)); +} // Make a default set of plugins with the latest Plugin version. In some tests // we'll shadow this `plugins` variable with one created with different versions. const plugins = makePlugins(Plugin); @@ -718,4 +721,178 @@ describe('Builder', function() { expect(Builder.prototype).to.have.nested.property('features.persistentOutputFlag', true); }); }); + + describe('cancel()', function() { + it('handles a cancel without an active build (has no affect)', function() { + let stepA = new plugins.Noop(); + let pipeline = new Builder(stepA); + + pipeline.cancel(); + pipeline.cancel(); // ensure double cancel is always safe here + + expect(stepA.buildCount).to.eql(0); + + return pipeline.build().then(() => { + expect(stepA.buildCount).to.eql(1); + }); + }); + + it('returns a promise which waits until cancellation is complete', function() { + let pipeline; + let stepAIsComplete = false; + let cancellingIsComplete = false; + + class StepA extends plugins.Deferred { + build() { + resolveCancel(pipeline.cancel()); + + setTimeout(() => this.resolve(), 50); + + return super.build().then(() => { + stepAIsComplete = true; + expect(cancellingIsComplete).to.eql( + false, + 'expected cancelling to not complete before stepA' + ); + }); + } + } + + const stepA = new StepA(); + const stepB = new plugins.Deferred([stepA]); + + pipeline = new Builder(stepB); + + const building = expect(pipeline.build()).to.eventually.be.rejectedWith('BUILD CANCELLED'); + + let resolveCancel; + const cancelling = new RSVP.Promise(resolve => (resolveCancel = resolve)); + + return RSVP.Promise.all([ + building, + cancelling.then(() => { + cancellingIsComplete = true; + expect(stepAIsComplete).to.eql( + true, + 'expected stepA to be complete before cancel completes' + ); + }), + ]); + }); + + it('errors if a new build is attempted during a cancellation', function() { + const stepA = new plugins.Deferred(); + const pipeline = new Builder(stepA); + const build = pipeline.build(); + + return wait().then(() => { + let wait = RSVP.Promise.all([ + expect(build).to.eventually.be.rejectedWith('BUILD CANCELLED'), + pipeline.cancel(), + expect(pipeline.build()).to.eventually.be.rejectedWith( + 'Cannot start a build if one is already running' + ), + expect(pipeline.build()).to.eventually.be.rejectedWith( + 'Cannot start a build if one is already running' + ), + expect(pipeline.build()).to.eventually.be.rejectedWith( + 'Cannot start a build if one is already running' + ), + ]); + + stepA.resolve(); + + return wait; + }); + }); + + it('build before cancel completes', function() { + let stepA = new plugins.Noop(); + let pipeline = new Builder(stepA); + + pipeline.cancel(); + pipeline.cancel(); // ensure double cancel is always safe here + + expect(stepA.buildCount).to.eql(0); + + return pipeline.build().then(() => { + expect(stepA.buildCount).to.eql(1); + }); + }); + + it('it cancels immediately if cancelled immediately after build', function() { + const step = new plugins.Deferred(); + let pipeline = new Builder(step); + step.resolve(); + let build = pipeline.build(); + pipeline.cancel(); + + return RSVP.Promise.resolve( + expect(build).to.eventually.be.rejectedWith('BUILD CANCELLED') + ).finally(() => { + expect(step.buildCount).to.eql(0); + }); + }); + + it('completes the current task before cancelling, and can be resumed', function() { + let pipeline; + + class SometimesBuildCanceller extends plugins.Noop { + build() { + super.build(); + + // cancel the first build, but allow the rest to proceed + if (this.buildCount === 1 || this.buildCount === 3) { + pipeline.cancel(); + } + } + } + + const stepA = new SometimesBuildCanceller(); + + const stepB = new plugins.Noop([stepA]); + const stepC = new plugins.Noop([stepB]); + + pipeline = new Builder(stepC); + + // build #1 + let build = pipeline.build(); + + // once the build has begun: + // 1. allow StepA to complete + // 2. cancel the build + // 3. wait for the build settle + // (stepB and stepC should not have run) + + return expect( + build.finally(() => { + expect(stepA.buildCount).to.eql(1, 'stepA.buildCount'); + expect(stepB.buildCount).to.eql(0, 'stepB.buildCount'); + expect(stepC.buildCount).to.eql(0, 'stepC.buildCount'); + }) + ) + .to.eventually.be.rejectedWith('BUILD CANCELLED') + .then(() => { + // build #2 + let build = pipeline.build(); + + return build.then(() => { + expect(stepA.buildCount).to.eql(2, 'stepA.buildCount'); + expect(stepB.buildCount).to.eql(1, 'stepB.buildCount'); + expect(stepC.buildCount).to.eql(1, 'stepC.buildCount'); + + // build #3 + return expect(pipeline.build()) + .to.eventually.be.rejectedWith('BUILD CANCELLED') + .then(() => { + // build will cancel again during stepA (before the stepB) so + // only stepA should have made progress + expect(stepA.buildCount).to.eql(3, 'stepA.buildCount'); + expect(stepB.buildCount).to.eql(1, 'stepB.buildCount'); + expect(stepC.buildCount).to.eql(1, 'stepC.buildCount'); + }); + }); + }); + }); + }); }); diff --git a/test/cancellation-request-test.js b/test/cancellation-request-test.js new file mode 100644 index 00000000..7ed061c2 --- /dev/null +++ b/test/cancellation-request-test.js @@ -0,0 +1,52 @@ +'use strict'; + +const chai = require('chai'); +const expect = chai.expect; +const chaiAsPromised = require('chai-as-promised'); +const CancelationRequest = require('../lib/cancellation-request'); +const RSVP = require('rsvp'); +const BuilderError = require('../lib/errors/builder'); + +chai.use(chaiAsPromised); + +describe('cancellation-request', function() { + it('.isCancelled / .cancel', function() { + let request = new CancelationRequest(RSVP.Promise.resolve()); + + expect(request.isCancelled).to.eql(false); + let wait = request.cancel(); + expect(request.isCancelled).to.eql(true); + + return wait.then(() => { + expect(request.isCancelled).to.eql(true); + }); + }); + + it('.throwIfRequested (requested)', function() { + let request = new CancelationRequest(RSVP.Promise.resolve()); + + request.throwIfRequested(); + + return request.cancel().then(() => { + expect(() => { + request.throwIfRequested(); + }).to.throw('BUILD CANCELLED'); + + expect(() => { + request.throwIfRequested(); + }).to.throw('BUILD CANCELLED'); + }); + }); + + it('.cancel (with builder rejection)', function() { + let request = new CancelationRequest(RSVP.Promise.reject(new BuilderError())); + + return request.cancel(); + }); + + it('.cancel (with non-builder rejection)', function() { + let request = new CancelationRequest(RSVP.Promise.reject(new Error('OOPS'))); + + return expect(request.cancel()).to.eventually.be.rejectedWith('OOPS'); + }); +}); diff --git a/test/plugins.js b/test/plugins.js index f3f5ab60..3f333c2c 100644 --- a/test/plugins.js +++ b/test/plugins.js @@ -9,36 +9,48 @@ const symlinkOrCopySync = require('symlink-or-copy').sync; module.exports = function(Plugin) { const plugins = {}; - plugins.Noop = class NoopPlugin extends Plugin { - build() {} - }; - - // This plugin writes foo.js into its outputPath - plugins.Veggies = class VeggiesPlugin extends Plugin { + class CountingPlugin extends Plugin { constructor(inputNodes, options) { super(inputNodes || [], options); + this._count = 0; } build() { + this._count++; + } + + get buildCount() { + return this._count; + } + } + + plugins.Noop = class NoopPlugin extends CountingPlugin {}; + + // This plugin writes foo.js into its outputPath + plugins.Veggies = class VeggiesPlugin extends CountingPlugin { + build() { + super.build(); fs.writeFileSync(this.outputPath + '/veggies.txt', 'tasty'); } }; - plugins.Merge = class MergePlugin extends Plugin { + plugins.Merge = class MergePlugin extends CountingPlugin { build() { + super.build(); for (let i = 0; i < this.inputPaths.length; i++) { symlinkOrCopySync(this.inputPaths[i], this.outputPath + '/' + i); } } }; - plugins.Failing = class FailingPlugin extends Plugin { + plugins.Failing = class FailingPlugin extends CountingPlugin { constructor(errorObject, options) { super([], options); this.errorObject = errorObject; } build() { + super.build(); throw this.errorObject; } }; @@ -47,7 +59,7 @@ module.exports = function(Plugin) { // The build will stall until you call node.finishBuild(). // To wait until the build starts, chain on node.buildStarted. // Don't build more than once. - plugins.Async = class AsyncPlugin extends Plugin { + plugins.Async = class AsyncPlugin extends CountingPlugin { constructor(inputNodes, options) { super(inputNodes || [], options); this.buildFinishedDeferred = RSVP.defer(); @@ -56,6 +68,7 @@ module.exports = function(Plugin) { } build() { + super.build(); this.buildStartedDeferred.resolve(); return this.buildFinishedDeferred.promise; } @@ -69,15 +82,31 @@ module.exports = function(Plugin) { } }; - plugins.Sleeping = class SleepingPlugin extends Plugin { + plugins.Sleeping = class SleepingPlugin extends CountingPlugin { constructor(inputNodes, options) { super(inputNodes || [], options); } build() { + super.build(); return new RSVP.Promise(resolve => setTimeout(resolve, 10)); } }; + plugins.Deferred = class DeferredPlugin extends CountingPlugin { + constructor(inputNodes, options) { + super(inputNodes || [], options); + this.promise = new RSVP.Promise((resolve, reject) => { + this.resolve = resolve; + this.reject = reject; + }); + } + + build() { + super.build(); + return this.promise; + } + }; + return plugins; };