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

(WIP) lib: fix beforeExit not working with -e #9680

Conversation

MylesBorins
Copy link
Contributor

@MylesBorins MylesBorins commented Nov 18, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test, lib

Description of change

This is an attempted backport of c5b07d4 to v4.x

It involved manually recreating the entire commit, as bootstrap_node.js does not exist in v4.x and the stack traces for the tests are quite different.

Unfortunately when running the tests we are seeing a failure in test/parallel/test-debug-brk.js. The node process for that test is not exiting and it is leaving phantom processes running 👻

Please do not run this in CI until we have figured out what is going on with the phantom processes.

@bnoordhuis do you have any idea why this is happening?

/cc @nodejs/lts

Commit 93a44d5 ("src: fix deferred events not working with -e") defers
evaluation of the script to the next tick.

A side effect of that change is that 'beforeExit' listeners run before
the actual script. 'beforeExit' is emitted when the event loop is
empty but process.nextTick() does not ref the event loop.

Fix that by using setImmediate(). Because it is implemented in terms
of a uv_check_t handle, it interacts with the event loop properly.

Fixes: #8534
PR-URL: #8821
Reviewed-By: Colin Ihrig cjihrig@gmail.com

@MylesBorins MylesBorins added lib / src Issues and PRs related to general changes in the lib or src directory. lts Issues and PRs related to Long Term Support releases. v4.x labels Nov 18, 2016
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. v4.x labels Nov 18, 2016
Commit 93a44d5 ("src: fix deferred events not working with -e") defers
evaluation of the script to the next tick.

A side effect of that change is that 'beforeExit' listeners run before
the actual script.  'beforeExit' is emitted when the event loop is
empty but process.nextTick() does not ref the event loop.

Fix that by using setImmediate().  Because it is implemented in terms
of a uv_check_t handle, it interacts with the event loop properly.

Ref: nodejs#9680
Fixes: nodejs#8534
PR-URL: nodejs#8821
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins force-pushed the backport-8821-fix-before-exit branch from 135218e to 25c3207 Compare November 18, 2016 19:59
@mscdex mscdex added the wip Issues and PRs that are still a work in progress. label Nov 18, 2016
@jasnell
Copy link
Member

jasnell commented Mar 1, 2017

ping @MylesBorins ... is this still in consideration for v4?

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Mar 1, 2017
@MylesBorins MylesBorins closed this Mar 9, 2017
@MylesBorins MylesBorins deleted the backport-8821-fix-before-exit branch November 14, 2017 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. lts Issues and PRs related to Long Term Support releases. stalled Issues and PRs that are stalled. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants