-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
process: flaky behavior of 'exit' event handler #12322
Comments
What about a timeout of 1, is that flaky? |
@sam-github For me, yes, it is. |
I think a timeout of 0 is the same as 1 due to this code. fwiw, I prefer (2), since (1) might make people think that there would be no flakiness, even if the timeout were lowered. |
I’m not sure, but isn’t it a bug that that code actually runs? |
Smells like a bug to me. |
@sam-github @addaleax Could we check this for various OS? I've checked for various Node.js versions in Windows 7 x64: Node.js 4.8.2 x64 (v8 4.5.103.46) Node.js 6.10.2 x64 (v8 5.1.281.98) Node.js 7.8.0 x64 (v8 5.5.372.43) Node.js 8.0.0-nightly201703249ff7ed23cd x64 (v8 5.6.326.57) Node.js 8.0.0-nightly20170404394b6ac5cb x64 (v8 5.7.492.69) Node.js 8.0.0-pre x64 (v8 5.8.202) Node.js 8.0.0-pre x64 (v8 5.9.0 candidate turbo on) Node.js 8.0.0-pre x64 (v8 5.9.0 candidate turbo off) |
I can confirm, the behaviour differs between Node 6 and Node 7 on x64 Linux, too. |
Does this only happen if you let the process expire normally, or also if you call
Lines 4383 to 4392 in affe0f2
|
ok I just double-checked and that doesn't make sense because your scheduling it in the exit handler... Smells like a bug but I'm not sure where |
/cc @bnoordhuis – This really seems to be the first commit where this happens, although I can’t quite make out why that is from looking at the diff… |
It's caused by the aac79df merges CleanupHandles() into the destructor, before that commit it was only used in debug-agent.cc. |
@bnoordhuis Thanks for explaining, makes sense. I would say this is a bug that we should fix by ensuring the “old” behaviour; do you agree? And if you do, would you prefer to do that yourself? I should be able to take care of it, too. |
Maybe a test could be added to check old behavior. |
Oh, yeah, definitely – we should have a test for that, no matter whether we change behaviour or not. One can turn your code into a non-flaky version like this: process.on('exit', (code) => {
setTimeout(() => {
console.log('This will not run'); // crash or w/e
}, 0);
// busy loop to make sure the timeout always expires during this tick
const a = process.hrtime();
while (process.hrtime(a)[1] < 5000);
}); |
It makes timers and other libuv handles fire intermittently after the 'exit' event, contrary to what the documentation states. Regression introduced in commit aac79df ("src: use stack-allocated Environment instances") from June last year that made the `while (handle_cleanup_waiting_ != 0) uv_run(event_loop(), UV_RUN_ONCE)` loop run unconditionally on exit because it merged CleanupHandles() into the Environment destructor. This change breaks parallel/test-async-wrap-throw-from-callback because the async_wrap idle handle is no longer cleaned up, which I resolved pragmatically by removing the test. In all seriousness, it is being removed in the upcoming async_wrap revamp - it doesn't make sense to sink a lot of time in it now. Fixes: nodejs#12322 PR-URL: nodejs#12344 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Fixed in 5ef6000. |
It makes timers and other libuv handles fire intermittently after the 'exit' event, contrary to what the documentation states. Regression introduced in commit aac79df ("src: use stack-allocated Environment instances") from June last year that made the `while (handle_cleanup_waiting_ != 0) uv_run(event_loop(), UV_RUN_ONCE)` loop run unconditionally on exit because it merged CleanupHandles() into the Environment destructor. This change breaks parallel/test-async-wrap-throw-from-callback because the async_wrap idle handle is no longer cleaned up, which I resolved pragmatically by removing the test. In all seriousness, it is being removed in the upcoming async_wrap revamp - it doesn't make sense to sink a lot of time in it now. Fixes: nodejs#12322 PR-URL: nodejs#12344 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Rename test-fs-truncate-nodejsGH-6233 to test-fs-truncate-clear-file-zero Rename test-process-exit-nodejsGH-12322 to test-process-exit-flaky-handler Refs: nodejs#19105 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure
- Rename test-fs-truncate-GH-6233 to test-fs-truncate-clear-file-zero - Rename test-process-exit-GH-12322 to test-process-exit-handler PR-URL: #19668 Refs: #19105 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
- Rename test-fs-truncate-GH-6233 to test-fs-truncate-clear-file-zero - Rename test-process-exit-GH-12322 to test-process-exit-handler PR-URL: #19668 Refs: #19105 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
- Rename test-fs-truncate-GH-6233 to test-fs-truncate-clear-file-zero - Rename test-process-exit-GH-12322 to test-process-exit-handler PR-URL: #19668 Refs: #19105 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
process.md
states:However, the behavior of the code is flaky:
If this flakiness is not a bug, what would be better?
1000
or something and save the categorical 'the timeout will never occur'.The text was updated successfully, but these errors were encountered: