Skip to content

Commit

Permalink
Merge pull request #350 from broccolijs/build-cancellation
Browse files Browse the repository at this point in the history
Add Build Cancellation
  • Loading branch information
rwjblue authored Mar 19, 2018
2 parents 50053f6 + 3f4aea2 commit 7d97eba
Show file tree
Hide file tree
Showing 8 changed files with 369 additions and 13 deletions.
36 changes: 33 additions & 3 deletions lib/builder.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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.
Expand All @@ -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`

Expand All @@ -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
Expand Down
37 changes: 37 additions & 0 deletions lib/cancellation-request.js
Original file line number Diff line number Diff line change
@@ -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;
});
}
};
3 changes: 3 additions & 0 deletions lib/errors/build.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down
5 changes: 5 additions & 0 deletions lib/errors/builder.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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;
}
};
23 changes: 23 additions & 0 deletions lib/errors/cancelation.js
Original file line number Diff line number Diff line change
@@ -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;
}
};
177 changes: 177 additions & 0 deletions test/builder_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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');
});
});
});
});
});
});
Loading

0 comments on commit 7d97eba

Please sign in to comment.