lib: fix beforeExit not working with -e - #8821
Conversation
Sorry, something went wrong.
|
patch LGTM, but isn't there still a larger issue that this is necessary? That is, normal modules don't seem to need this, so why does -e? |
Sorry, something went wrong.
Fishrock123
left a comment
There was a problem hiding this comment.
Actually, the test is timing out on Windows: https://ci.nodejs.org/job/node-test-binary-windows/4085/RUN_SUBSET=1,VS_VERSION=vs2015,label=win2012r2/tapTestReport/test.tap-28/
Sorry, something went wrong.
Sorry, something went wrong.
|
@nodejs/platform-windows Any ideas on why the test times out on Windows? Example: https://ci.nodejs.org/job/node-test-binary-windows/4154/RUN_SUBSET=1,VS_VERSION=vcbt2015,label=win10/console |
Sorry, something went wrong.
|
Investigated, but this was not easy as it first seemed to be. The test hangs in a live loop printing beforeExit to stdout. Apparently it's an unrelated issue, opened #9067 with my findings. |
Sorry, something went wrong.
|
As shown in #9067 , using console.log in beforeExit creates a loop. @bnoordhuis perhaps change the test to something like const script = `
var beforeExitCalled = false;
process.on("beforeExit", () => {beforeExitCalled = true});
process.on("exit", () => {if(beforeExitCalled) console.log("ok")});
`;(This seems to work on Windows: fails without and passes with the change on this PR.) |
Sorry, something went wrong.
|
Aw, and I even commented on that issue. I must have been having a brain boo boo that day. Thanks for the pointer. Rebased and updated, PTAL. I switched the beforeExit listener from .on() to .once(). |
Sorry, something went wrong.
|
Green! (Well, yellow.) @cjihrig @Fishrock123 Still LGTY?
The module system does it too, see Module.runMain(). The equivalent for -e would be this: diff --git a/lib/internal/bootstrap_node.js b/lib/internal/bootstrap_node.js
index 8b8d066..e778cb4 100644
--- a/lib/internal/bootstrap_node.js
+++ b/lib/internal/bootstrap_node.js
@@ -338,13 +338,9 @@
'return require("vm").runInThisContext(' +
`${JSON.stringify(body)}, { filename: ` +
`${JSON.stringify(name)}, displayErrors: true });\n`;
- // Defer evaluation for a tick. This is a workaround for deferred
- // events not firing when evaluating scripts from the command line,
- // see https://github.com/nodejs/node/issues/1600.
- process.nextTick(function() {
- const result = module._compile(script, `${name}-wrapper`);
- if (process._print_eval) console.log(result);
- });
+ const result = module._compile(script, `${name}-wrapper`);
+ if (process._print_eval) console.log(result);
+ process._tickCallback();
}
// Load preload modulessetImmediate() and process._tickCallback() are functionally identical here as far as I can tell |
Sorry, something went wrong.
|
Not sure if the template string in the test should be indented 2 or 4 spaces, but yes, still LGTM. |
Sorry, something went wrong.
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: nodejs#8534 PR-URL: nodejs#8821 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
I see both styles in the code base and the linter isn't complaining so I decided to leave it as-is. |
Sorry, something went wrong.
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>
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>
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>
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>
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
R=@Fishrock123