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

POC [Fixes #311] fix cleanup #312

Closed
wants to merge 1 commit into from
Closed

Conversation

stefanpenner
Copy link
Contributor

  • ensure cleanup gives the ability to wait until it is done
  • improves support for mid-build cleanup, waits for the current task to complete, prevents the next task, and then exists.

* ensure cleanup gives the ability to wait until it is done
* improves support for mid-build cleanup, waits for the current task to complete, prevents the next task, and then exists.
}

// Destructor-like method. Cleanup is synchronous at the moment, but in the
// future we might change it to return a promise.
Builder.prototype.cleanup = function() {
this._canceled = true
this.builderTmpDirCleanup()
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's correct to try removing tmp dirs with builderTmpDirCleanup while the build is still in progress. We'd need to wait for the current step to finish first.

Copy link
Member

Choose a reason for hiding this comment

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

Oh. Wait, I think you're already saying this in your original comment (your list is a to-do list, yes?)...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya, the version in broccoli-builder 0.18.x waits

@@ -87,14 +91,17 @@ Builder.prototype.build = function() {

promise = promise
.then(function() {
if (self.cancelled) { return }
Copy link
Member

Choose a reason for hiding this comment

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

Tests will catch this anyway, but there's a tyop here (self._canceled) :)

@joliss
Copy link
Member

joliss commented Dec 3, 2016

I actually wonder if it's conceptually necessary to tie the cancellation thing to the cleanup function. In other words, I don't think there's any reason why in general we couldn't start a fresh build after canceling the current build, instead of cleaning up immediately. We shouldn't end up in any "wedged" state that would stop us from doing so.

So if we do that, the Ctrl+C handler would do something like:

cancelCurrentBuild()
then wait for currentBuild
then cleanup

@stefanpenner
Copy link
Contributor Author

stefanpenner commented Dec 4, 2016

@joliss i'll update this to be more like what I ended-up with on 0.18.x. There are some nuanced differences, that have not made it to this PR yet.


Some thoughts:

  • adding a public cancel independent of cleanup is totally fine, will gladly add that. We can also make an non exceptional cancel recoverable.
  • cleanup should likely still call cancel, otherwise cleanup and an running build will simply race. We can also make a non exceptional cleanup recoverable
  • we need to decide the ordering between the completion of cancel() cleanup() and builder.build(). I believe builder.build completion must wait for any pending cleanup() which in tern must wait for any pending cancel(). That way the external world remains observably consistent, regardless if a build completes via rejection due to cancellation, due to error, or totally normal runtime.
  • the system should not allow any more then 1 build, 1 cleanup and 1 cancel at any point in time, and they must correctly wait on each other to ensure the build remains observably consistent.

@stefanpenner
Copy link
Contributor Author

replaced by: #350

@stefanpenner stefanpenner deleted the mid-build-cleanup branch December 22, 2017 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants