Add Build Cancellation #350
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stefanpenner
commented
Dec 12, 2017
lib/builder.js
Outdated
| @@ -81,8 +83,14 @@ 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('WTF'); | |||
There was a problem hiding this comment.
better error message, likely a BuildError with a message similar tocannot build mid-build
lib/cancellation-request.js
Outdated
|
|
||
| const CancelationError = require('./errors/cancelation'); | ||
|
|
||
| module.exports = class CancelationRequest { |
There was a problem hiding this comment.
this class needs to be unit tested
rwjblue
reviewed
Dec 12, 2017
| @@ -29,6 +29,9 @@ RSVP.on('error', error => { | |||
| throw error; | |||
| }); | |||
|
|
|||
| function wait(time) { | |||
There was a problem hiding this comment.
I like calling this settled because await wait() is weird 😝
There was a problem hiding this comment.
i think this is more nextTick
stefanpenner
commented
Dec 12, 2017
test/builder_test.js
Outdated
| let resolveCancel; | ||
| const cancelling = new RSVP.Promise(resolve => (resolveCancel = resolve)); | ||
|
|
||
| stepA.build = function() { |
There was a problem hiding this comment.
subclass rather then hackity hack
chrmod
reviewed
Dec 12, 2017
stefanpenner
force-pushed
the
build-cancellation
branch
3 times, most recently
from
757d2fc to
527f597
Compare
``` 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.
stefanpenner
force-pushed
the
build-cancellation
branch
from
527f597 to
3f4aea2
Compare
This was referenced Dec 22, 2017
|
@stefanpenner what’s the state of play with this? Looks pretty good rn |
rwjblue
approved these changes
Mar 19, 2018
|
@oligriffiths / @stefanpenner - Lets move the remaining TODO's:
|
This was referenced Apr 3, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Descrption
For larger builds, it is important that a mid-build
<ctrl-c>can both gracefully exit, but also exist as fast as possible. In-order to accomplish this, we need to be able to terminate the build after the current step completes.Also for larger builds, it is important that if a new change event occurs mid-build, that we can pause the build after the current step, and resume it from the beginning (in a safe way). This provides a good user-experience, as 2 changes slower then the throttle threshold but faster then a rebuild will still only produce 1 consistent output. No accidentally mid-build stale outputs.
TODOS
cancelguarantees to leave the build in a resumable statecancelsuggests a unrecoverable buildbuilder.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.builder.buid()is not allowed if thebuildercurrently has a pending build or cancellationbuilder.cancelis idempotentisSilentandisCancellationto play nicely with consumers like ember-cliTODO: