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

node 20.1.0 --loader seems trigger beforeExit when use await import #47929

Closed
wacdev opened this issue May 9, 2023 · 3 comments · Fixed by #47964
Closed

node 20.1.0 --loader seems trigger beforeExit when use await import #47929

wacdev opened this issue May 9, 2023 · 3 comments · Fixed by #47964
Labels
confirmed-bug Issues with confirmed bugs. loaders Issues and PRs related to ES module loaders v20.x v20.x Issues that can be reproduced on v20.x or PRs targeting the v20.x-staging branch.

Comments

@wacdev
Copy link

wacdev commented May 9, 2023

Version

20.1.0

Platform

macos

What do you see instead?

My module will close the redis connection at the time of beforeExit, and the await import module (19.9.0) will not trigger beforeExit before.

In addition, the exit event is not triggered when use await import module

@MoLow MoLow added confirmed-bug Issues with confirmed bugs. loaders Issues and PRs related to ES module loaders v20.x v20.x Issues that can be reproduced on v20.x or PRs targeting the v20.x-staging branch. labels May 9, 2023
@MoLow
Copy link
Member

MoLow commented May 9, 2023

also reported here - with a minimal repro

// file.js
process.on('beforeExit', () => {
  console.log('beforeExit')
})
import('./anyfile.js')
console.log('THIS CONSOLE LOG IS PREINTED')
./node  file.js                                                                                              ✔
THIS CONSOLE LOG IS PREINTED
beforeExit
./node --experimental-loader="data:text/javascript,export default true"  file.js                             ✔
(node:62415) ExperimentalWarning: Custom ESM Loaders is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
THIS CONSOLE LOG IS PREINTED
beforeExit
beforeExit
beforeExit

@targos
Copy link
Member

targos commented May 10, 2023

This is probably because the ESM loader uses process.on('beforeExit') in a way that asynchronous work is scheduled in the callback. We should probably refactor that part anyway, I don't like that the internals use a public API that can be tampered with (for example with process.removeAllListeners('beforeExit');)

Refs: https://nodejs.org/dist/latest-v20.x/docs/api/process.html#event-beforeexit

/cc @aduh95

@aduh95
Copy link
Contributor

aduh95 commented May 10, 2023

The underlying issue being that Atomics.waitAsync does not keep the event loop alive, which has been reported to them already. That being said, we should be able to work around this without triggering beforeExit events on the main thread (on the loader thread though, I don’t think we can work around it, and calling process.removeAllListeners('beforeExit') from the loader thread needs to be forbidden probably.

nodejs-github-bot pushed a commit that referenced this issue May 14, 2023
PR-URL: #47964
Fixes: #47929
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
targos pushed a commit that referenced this issue May 14, 2023
PR-URL: #47964
Fixes: #47929
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
targos pushed a commit that referenced this issue May 15, 2023
PR-URL: #47964
Fixes: #47929
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
targos pushed a commit to targos/node that referenced this issue Nov 11, 2023
PR-URL: nodejs#47964
Fixes: nodejs#47929
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
targos pushed a commit that referenced this issue Nov 23, 2023
PR-URL: #47964
Backport-PR-URL: #50669
Fixes: #47929
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
sercher added a commit to sercher/graaljs that referenced this issue Apr 25, 2024
PR-URL: nodejs/node#47964
Backport-PR-URL: nodejs/node#50669
Fixes: nodejs/node#47929
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
sercher added a commit to sercher/graaljs that referenced this issue Apr 25, 2024
PR-URL: nodejs/node#47964
Backport-PR-URL: nodejs/node#50669
Fixes: nodejs/node#47929
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. loaders Issues and PRs related to ES module loaders v20.x v20.x Issues that can be reproduced on v20.x or PRs targeting the v20.x-staging branch.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants