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

Re-implement subset of ThreadSafeCallback using thread safe function from napi #219

Merged
merged 2 commits into from
Sep 25, 2021

Conversation

enami
Copy link

@enami enami commented Aug 10, 2021

This PR implements subset of ThreadSafeCallback class, originally provided by napi-thread-safe-callback npm, on the top of thread safe function provided by node api, expecting to fix issue #215

@enami
Copy link
Author

enami commented Aug 10, 2021

When I run noble with electron, it segmentation faults on quit as issue #215 describe.

If I run with this patch, ...
diff6.txt

... the output looks like this:

$ ./node_modules/.bin/electron .
Impl()
env 0x7ffbabc3ad00: !empty
0x10cf93fa8
0x3e00000000
~Emit()
env 0x7ffbabc3ad00: !empty
0x10cf93ff8
0x3e00000000
~ThreadSafeCallback()
env 0x7ffbabc3ad00: !empty
0x10cf93ff8
0x3e00000000
~Impl()
env 0x7ffbabc3ad00: !empty
0x8000000000000000
0x400007ffbabcf8e1
/private/tmp/noble-electron-crash-sample/node_modules/electron/dist/Electron.app/Contents/MacOS/Electron exited with signal SIGSEGV

So, it looks like memory region pointed by env is already modified by someone else when ThreadSafeCallback::~Impl() is called. Since the node document says that The napi_env becomes invalid when an instance of a native addon is unloaded. , I suspect a module is unloaded before libuv thread calls ThreadSafeCallback::~Impl().

The crash occurs on OSX big sur, but not on catalina. I'm not sure why but possibly due to difference of implementation details between OSes.

I guess it would be possible to modify original ThreadSafeCallback to delete node derived objects eariler, but at the same time we need to care so that threads won't touch deleted objects.

Since node api provides same functionality as thread safe function nowadays, I hope such complicated works are done inside node api library. So I've created this PR and at least it works for me.

@enami enami mentioned this pull request Aug 10, 2021
@rzr
Copy link

rzr commented Aug 10, 2021

Is there a related issue reported in NAPI ?

@enami
Copy link
Author

enami commented Aug 10, 2021

Do you really mean napi rather than napi-thread-safe-callback? Anyway, I've just searched briefly in node or node-addon-api issues but I couldn't find something similar or related.

@rzr
Copy link

rzr commented Aug 11, 2021

It would make sense to try to suggest this feature upstream and see how it goes, then I'll merge this workaround patch

@enami
Copy link
Author

enami commented Aug 11, 2021

Then, an upstream in this case is not napi but napi-thread-safe-callback npm.

@rzr
Copy link

rzr commented Aug 16, 2021

yes if you can comment elsewhere and link back here it can be helpful to community

@enami
Copy link
Author

enami commented Sep 1, 2021

Here is a comment from author of napi-thread-safe-callback: mika-fischer/napi-thread-safe-callback#14 (comment)

@rzr rzr merged commit ae2f3dd into abandonware:master Sep 25, 2021
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.

None yet

2 participants