n-api: regression with napi_delete_async_work since v10.2.0 #20966
Comments
|
@MayhemYDG can you create a minimum test case to illustrate the problem? |
|
@gabrielschulhof I'm trying to create a minimum test case from scratch, but it's not failing unfortunately... |
|
I think I've reproduced it.
…On Mon, Jun 4, 2018 at 3:29 PM, Nicolas Stepien ***@***.***> wrote:
@gabrielschulhof <https://github.com/gabrielschulhof> I'm trying to
create a minimum test case from scratch, but it's not failing...
There must be more to it but I don't know where to look. It only happens
on Node 10.2.0+.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#20966 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AA7k0RY5TBE7pP-aXNvLegEQFUgTzPf_ks5t5YqRgaJpZM4UOZsE>
.
|
|
@MayhemYDG https://gist.github.com/gabrielschulhof/4d01d88a75df33981511304162c4a063 crashes when built against master, but runs fine prior to commit c072057. |
|
This is a bug we've fixed before. Before calling the |
|
Actually, we've fixed it before, but not with async work: 1a5a19d 🙂 |
We must back up the value of `_env` before calling the async work complete callback, because the complete callback may delete the instance in which `_env` is stored by calling `napi_delete_async_work`, and because we need to use it after the complete callback has completed. Fixes: nodejs#20966
We must back up the value of `_env` before calling the async work complete callback, because the complete callback may delete the instance in which `_env` is stored by calling `napi_delete_async_work`, and because we need to use it after the complete callback has completed. Fixes: #20966 PR-URL: #21129 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
|
Are you sure this fix resolved everything? Because my simple test (https://github.com/s-h-a-d-o-w/napi-test) still crashes at the call to |
|
@s-h-a-d-o-w I'll try to reproduce the crash. |
|
@s-h-a-d-o-w at https://github.com/s-h-a-d-o-w/napi-test/blob/master/src/napi_init.cc#L103-L106 you assign If you move |
|
Ahhh! Thanks! |
I'm experimenting with n-api in a branch of iltorb: nstepien/iltorb#70
I added calls to
napi_delete_async_workafter forgetting to make use of it, but I ended up getting rogue segmentation faults.The tests pass on previous versions of Node on various CI, and also on Node
v10.1.0locally, but not Nodev10.2.0.Removing the calls to
napi_delete_async_workseems to appease Nodev10.2.x.The text was updated successfully, but these errors were encountered: