Skip to content

fix napi_async_work prevent Node.js from exiting #26

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

Merged
merged 12 commits into from
Jan 20, 2023

Conversation

toyobayashi
Copy link
Owner

fixes #25

@toyobayashi
Copy link
Owner Author

toyobayashi commented Jan 17, 2023

seems this breaks cleanup hooks at process beforeExit event ...

@toyobayashi
Copy link
Owner Author

toyobayashi commented Jan 19, 2023

immediately call napi_cancel_async_work after napi_queue_async_work may cause error, in random probability

emnapi worker pool size in test is too great

@toyobayashi toyobayashi merged commit e98f300 into main Jan 20, 2023
@RReverser
Copy link
Contributor

The worker ref/unref logic here seems a bit complicated TBH, especially with various checks, postMessage and so on.

Node.js doesn't care which handle keeps the process alive, so you don't need to .ref() all individual workers when running an async operation - the MessagePort from a71cc25 should be enough.

From worker side, it should be enough to only .unref() workers once when an emnapi thread is created, and leave the rest of ref/unref to that MessagePort. I think that would simplify the code quite a bit.

@toyobayashi
Copy link
Owner Author

Done.

@RReverser
Copy link
Contributor

Nice, looks much simpler!

@RReverser
Copy link
Contributor

By the way, I wanted to ask - why did you need worker = worker.worker || worker? Isn't PThread.pthreads[id] always a Worker already?

@toyobayashi
Copy link
Owner Author

This is for backward compatibility, I remember emscripten once refactored PThread.

emscripten-core/emscripten#17492

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AsyncWorkers prevent Node.js from exiting
2 participants