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

repl: event ordering: delay 'close' until 'flushHistory' #3435

Merged
merged 1 commit into from
Oct 22, 2015

Conversation

Fishrock123
Copy link
Contributor

Emitting close before the history has flushed is somewhat incorrect and
rather confusing.

Fixes an issue related to #2356

Separate PR since I'm going to try to make #2356 work without it, but would already like review here.

@Fishrock123 Fishrock123 added repl Issues and PRs related to the REPL subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Oct 19, 2015
@Fishrock123 Fishrock123 added this to the 5.0.0 milestone Oct 19, 2015

return;
}
Interface.prototype.close.call(this);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe this should be called in nextTick?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would think so

@Fishrock123
Copy link
Contributor Author

@evanlucas
Copy link
Contributor

LGTM if CI is happy

@rvagg rvagg mentioned this pull request Oct 21, 2015
13 tasks
@evanlucas
Copy link
Contributor

so for failures we have:

  • test-child-process-fork-net2.js (known to be flaky I believe)
  • both plinux machines have failures (which I believe have been happening already)
  • a couple windows failures (which don't seem related)

I don't think any of these are related to the proposed changes. Still LGTM

@Fishrock123
Copy link
Contributor Author

cc @nodejs/tsc

@trevnorris
Copy link
Contributor

Code change LGTM. Just squash the commits.

Emitting 'close' before the history has flushed is somewhat incorrect
and rather confusing.

This also makes the 'close' event always asynchronous for consistency.

Refs: nodejs#2356
PR-URL: nodejs#3435
Reviewed By: Evan Lucas <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
@Fishrock123 Fishrock123 merged commit ecab7a6 into nodejs:master Oct 22, 2015
Fishrock123 added a commit that referenced this pull request Oct 22, 2015
Emitting 'close' before the history has flushed is somewhat incorrect
and rather confusing.

This also makes the 'close' event always asynchronous for consistency.

Refs: #2356
PR-URL: #3435
Reviewed By: Evan Lucas <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
@rvagg rvagg mentioned this pull request Oct 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repl Issues and PRs related to the REPL subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants