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

Prevent releaseExit while exiting. #22

Closed
wants to merge 1 commit into from

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Feb 19, 2017

The following roughly emulates what ember-cli is doing (2.13 canary only):

var captureExit = require('capture-exit');
var RSVP = require('rsvp');

var interruptHandler;
var promise = RSVP.Promise.resolve().
    then(function() {
      captureExit.captureExit();
      captureExit.onExit(interruptHandler);
    }).
    then(function() {
      // somewhere along the promise chain someone does:
      process.exit(1);
    }).
    then(function() {
      captureExit.releaseExit();
    }).
    then(function() {
      // later someone does:
      process.exit();
    });

interruptHandler = function() {
  return promise;
};

If we allow .releaseExit to run during the ._flush process, we can no longer ensure that the rest of the handlers are ran (if a process.exit() is ran after exit has been released).

See longer explaination / code safari in ember-cli/ember-cli#6779 (comment).


This seems good to me, but I'd love feedback from @stefanpenner / @hjdivad / and others as there may be alternative conclusions to be made from the details in ember-cli/ember-cli#6779.

The following roughly emulates what ember-cli is doing (2.13 canary only):

```
'use strict';

// capture-exit-onExit.js
var captureExit = require('.');

var RSVP = require('rsvp');

var interruptHandler;
var promise = RSVP.Promise.resolve().
    then(function() {
      console.log('capturing exit');
      captureExit.captureExit();
      captureExit.onExit(interruptHandler);
    }).
    then(function() {
      console.log('calling process.exit(1)');
      process.exit(1);
    }).
    then(function() {
      console.log('releasing exit');
      captureExit.releaseExit();
    }).
    then(function() {
      console.log('calling process.exit()');
      process.exit();
    });

interruptHandler = function() {
  console.log('interrupt handler running');
  return promise;
};
```

If we allow `.releaseExit` to run _during_ the `._flush` process, we can
no longer ensure that the rest of the handlers are ran (if a `process.exit()`
is ran after exit has been released).
@rwjblue
Copy link
Member Author

rwjblue commented Feb 19, 2017

I have confirmed (via npm link) that this does fix the issue identified in ember-cli/ember-cli#6779.

@stefanpenner
Copy link
Collaborator

Should we prevent, or should we error. releaseExit during exit, gives me a headache.

@hjdivad
Copy link
Contributor

hjdivad commented Feb 21, 2017

I'm 👍 on erroring here. releaseExit while exiting is always a bug no?

@rwjblue
Copy link
Member Author

rwjblue commented Feb 21, 2017

@hjdivad:

I'm 👍 on erroring here. releaseExit while exiting is always a bug no?

Well, it is the default case for ember-cli right now (as explained in ember-cli/ember-cli#6779 (comment)).

@rwjblue
Copy link
Member Author

rwjblue commented Feb 21, 2017

I am happy to change it around to be an error if isExiting though. In general (to me at least), releaseExit seems like it should almost exclusively be a test only "feature", and in normal production usage I do not think it should be used...

@stefanpenner
Copy link
Collaborator

stefanpenner commented Feb 21, 2017

releaseExit seems like it should almost exclusively be a test only "feature", and in normal production usage I do not think it should be used...

yes that was the intention. And specifically, for capture-exit's own tests.

@rwjblue
Copy link
Member Author

rwjblue commented Feb 21, 2017

Awesome, thanks for helping me understand the intent here.

I've submitted ember-cli/ember-cli#6799 to remove the releaseExit usage in ember-cli, and once that lands I'll update this PR to make it a hard error.

homu added a commit to ember-cli/ember-cli that referenced this pull request Feb 21, 2017
Refactor `capture-exit` usage to avoid releasing exit.

Calling `exit.releaseExit()` (possibly _while exiting_) will lead to very difficult to track down bugs.

We should capture exit very early in our processing, and simply not release. There are no downsides to this, and calling `captureExit.releaseExit()` during exit will soon throw an error (see ember-cli/capture-exit#22 (comment)).

Fixes #6779
@rwjblue rwjblue closed this Feb 21, 2017
@rwjblue rwjblue deleted the tweak-release-exit branch February 21, 2017 20:37
@rwjblue
Copy link
Member Author

rwjblue commented Feb 21, 2017

Closing, I'll open up a separate PR to make .releaseExit() throw if called while exiting.

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

3 participants