Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Build Cancellation #350

Merged
merged 1 commit into from
Mar 19, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like calling this settled because await wait() is weird 😝

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this is more nextTick

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