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

[0.70] Fix task runner for Node-API (#153) #169

Merged
merged 2 commits into from
Apr 26, 2023

Conversation

vmoroz
Copy link
Member

@vmoroz vmoroz commented Apr 26, 2023

Cherry pick PR #153 to the main branch.

Original PR description:

Currently the Node-API foreground task runner lifetime is depending on the napi_env lifetime.
This is wrong because some tasks must be run after the napi_env is destroyed.
This issue causes a crash in React Native for Windows when we do direct debugging.

In this PR we are fixing this issue by implementing a new task runner:

  • Client code must call v8_create_task_runner function to create a new task runner and then pass it to napi_ext_env_settings when creating new napi_env. The code that internally uses v8runtime::JSITaskRunner is responsible for deleting it.
  • The v8_create_task_runner captures external task scheduler and functions to run tasks and to destroy the external task scheduler.
  • This is a PR in RNW that implements the new API.

An additional change in this PR: removed NAPI_EXTERN from all implementations as it may cause issues if the function signature is not the same in the .h and .cpp files. This issue was observed while working on the new v8_create_task_runner function.

Note that the new code has prefix v8_ instead of previously used napi_ext_. The reason is that these functions are V8 JS Engine specific and will be never accepted as a part of the Node-API in Node.JS. Thus, the proposal is to use the v8_ prefix instead. Ideally, we should replace js_native_ext_api.h with a new v8_api.h where all declarations will have the v8_ prefix.

Microsoft Reviewers: Open in CodeFlow

@vmoroz vmoroz requested a review from tudorms April 26, 2023 18:33
@vmoroz vmoroz requested a review from a team as a code owner April 26, 2023 18:33
@vmoroz vmoroz merged commit 2053d41 into microsoft:0.70-stable Apr 26, 2023
@vmoroz vmoroz deleted the PR/0.70-FixTaskRunner branch April 26, 2023 22:02
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.

2 participants