🐛 fix hanging shutdown #188
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
also fixed multiple occasions where errorCallback must be set
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
possible fix for #187
The problem - as I understand it - seems to be that the callbacks are used in multiple threads.
First the constructor has the event and error callback as argument.
This is - as far as I understand it - the main thread. The initial usage count for this thread is 1.
Then at some point a watcher is started and the callback is actually used for the first time. This is done in a separate thread it seems.
https://github.com/nodejs/node-addon-api/blob/main/doc/threadsafe_function.md#acquire indicates that it tracks the usage per thread but a bit unclear to me.
So the worker threads acquire the functions and register that another thread is using them.
now when the script in the bug report executes nodejs has to think in the end that the mainthread is still using the callback and will not call the destructor of the object because there is no connection between the stop() function and the object itself. It might possibly be restarted again.
I added a super helpless close() function which Releases the function from the main thread :S
during debugging I also found multiple occasions where mErrorCallback is assumed to be present which is not enforced! I added a few ifs() there.
I am pretty sure this is not a good fix. but everything else would probably involve api changes :S