Skip to content

Commit 390f607

Browse files
gabrielschulhofgabrielschulhof
committed
src: restore ability to run under NAPI_EXPERIMENTAL
Since we made the default for Node.js core finalizers synchronous for users running with `NAPI_EXPERIMENTAL` and introduced `env->CheckGCAccess()` in Node.js core, we must now defer all finalizers in node-addon-api, because our users likely make non-gc-safe Node-API calls from existing finalizers. To that end, * Use the NAPI_VERSION environment variable to detect whether `NAPI_EXPERIMENTAL` should be on, and add it to the defines if `NAPI_VERSION` is set to `NAPI_VERSION_EXPERIMENTAL`, i.e. 2147483647. * When building with `NAPI_EXPERIMENTAL`, * render all finalizers asynchronous, and * expect `napi_cannot_run_js` instead of `napi_exception_pending`.
1 parent 7c79c33 commit 390f607

File tree

5 files changed

+130
-65
lines changed

5 files changed

+130
-65
lines changed

‎.github/workflows/ci.yml‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,14 @@ jobs:
1313
timeout-minutes: 30
1414
strategy:
1515
matrix:
16-
node-version: [ 18.x, 20.x, 21.x, 22.x ]
16+
api_version:
17+
- standard
18+
- experimental
19+
node-version:
20+
- 18.x
21+
- 20.x
22+
- 21.x
23+
- 22.x
1724
os:
1825
- macos-latest
1926
- ubuntu-latest
@@ -43,6 +50,9 @@ jobs:
4350
npm install
4451
- name: npm test
4552
run: |
53+
if [ "${{ matrix.api_version }}" = "experimental" ]; then
54+
export NAPI_VERSION=2147483647
55+
fi
4656
if [ "${{ matrix.compiler }}" = "gcc" ]; then
4757
export CC="gcc" CXX="g++"
4858
fi

‎common.gypi‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
},
66
'conditions': [
77
['NAPI_VERSION!=""', { 'defines': ['NAPI_VERSION=<@(NAPI_VERSION)'] } ],
8+
['NAPI_VERSION==2147483647', { 'defines': ['NAPI_EXPERIMENTAL'] } ],
89
['disable_deprecated=="true"', {
910
'defines': ['NODE_ADDON_API_DISABLE_DEPRECATED']
1011
}],

‎napi-inl.h‎

Lines changed: 116 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,16 @@
1818
#include <type_traits>
1919
#include <utility>
2020

21+
// TODO(gabrielschulhof): Remove this and remove the wrapping at the call sites
22+
// (i.e., at the call site, `TSFN_FINALIZER(x)` should be changed back to `x`)
23+
// after https://github.com/nodejs/node/pull/51801 has landed.
24+
#if (defined(NAPI_EXPERIMENTAL) && \
25+
defined(NODE_API_EXPERIMENTAL_HAS_POST_FINALIZER))
26+
#define TSFN_FINALIZER(fini) reinterpret_cast<node_api_nogc_finalize>(fini)
27+
#else
28+
#define TSFN_FINALIZER(fini) fini
29+
#endif
30+
2131
namespace Napi {
2232

2333
#ifdef NAPI_CPP_CUSTOM_NAMESPACE
@@ -31,6 +41,23 @@ namespace details {
3141
// Node.js releases. Only necessary when they are used in napi.h and napi-inl.h.
3242
constexpr int napi_no_external_buffers_allowed = 22;
3343

44+
#if (defined(NAPI_EXPERIMENTAL) && \
45+
defined(NODE_API_EXPERIMENTAL_HAS_POST_FINALIZER))
46+
template <napi_finalize finalizer>
47+
inline void PostFinalizerWrapper(node_api_nogc_env nogc_env,
48+
void* data,
49+
void* hint) {
50+
napi_status status = node_api_post_finalizer(nogc_env, finalizer, data, hint);
51+
NAPI_FATAL_IF_FAILED(
52+
status, "PostFinalizerWrapper", "node_api_post_finalizer failed");
53+
}
54+
#else
55+
template <napi_finalize finalizer>
56+
inline void PostFinalizerWrapper(napi_env env, void* data, void* hint) {
57+
finalizer(env, data, hint);
58+
}
59+
#endif
60+
3461
template <typename FreeType>
3562
inline void default_finalizer(napi_env /*env*/, void* data, void* /*hint*/) {
3663
delete static_cast<FreeType*>(data);
@@ -65,7 +92,8 @@ inline napi_status AttachData(napi_env env,
6592
}
6693
}
6794
#else // NAPI_VERSION >= 5
68-
status = napi_add_finalizer(env, obj, data, finalizer, hint, nullptr);
95+
status = napi_add_finalizer(
96+
env, obj, data, details::PostFinalizerWrapper<finalizer>, hint, nullptr);
6997
#endif
7098
return status;
7199
}
@@ -1774,7 +1802,8 @@ inline External<T> External<T>::New(napi_env env,
17741802
napi_status status =
17751803
napi_create_external(env,
17761804
data,
1777-
details::FinalizeData<T, Finalizer>::Wrapper,
1805+
details::PostFinalizerWrapper<
1806+
details::FinalizeData<T, Finalizer>::Wrapper>,
17781807
finalizeData,
17791808
&value);
17801809
if (status != napi_ok) {
@@ -1797,7 +1826,8 @@ inline External<T> External<T>::New(napi_env env,
17971826
napi_status status = napi_create_external(
17981827
env,
17991828
data,
1800-
details::FinalizeData<T, Finalizer, Hint>::WrapperWithHint,
1829+
details::PostFinalizerWrapper<
1830+
details::FinalizeData<T, Finalizer, Hint>::WrapperWithHint>,
18011831
finalizeData,
18021832
&value);
18031833
if (status != napi_ok) {
@@ -1910,7 +1940,8 @@ inline ArrayBuffer ArrayBuffer::New(napi_env env,
19101940
env,
19111941
externalData,
19121942
byteLength,
1913-
details::FinalizeData<void, Finalizer>::Wrapper,
1943+
details::PostFinalizerWrapper<
1944+
details::FinalizeData<void, Finalizer>::Wrapper>,
19141945
finalizeData,
19151946
&value);
19161947
if (status != napi_ok) {
@@ -1935,7 +1966,8 @@ inline ArrayBuffer ArrayBuffer::New(napi_env env,
19351966
env,
19361967
externalData,
19371968
byteLength,
1938-
details::FinalizeData<void, Finalizer, Hint>::WrapperWithHint,
1969+
details::PostFinalizerWrapper<
1970+
details::FinalizeData<void, Finalizer, Hint>::WrapperWithHint>,
19391971
finalizeData,
19401972
&value);
19411973
if (status != napi_ok) {
@@ -2652,13 +2684,14 @@ inline Buffer<T> Buffer<T>::New(napi_env env,
26522684
details::FinalizeData<T, Finalizer>* finalizeData =
26532685
new details::FinalizeData<T, Finalizer>(
26542686
{std::move(finalizeCallback), nullptr});
2655-
napi_status status =
2656-
napi_create_external_buffer(env,
2657-
length * sizeof(T),
2658-
data,
2659-
details::FinalizeData<T, Finalizer>::Wrapper,
2660-
finalizeData,
2661-
&value);
2687+
napi_status status = napi_create_external_buffer(
2688+
env,
2689+
length * sizeof(T),
2690+
data,
2691+
details::PostFinalizerWrapper<
2692+
details::FinalizeData<T, Finalizer>::Wrapper>,
2693+
finalizeData,
2694+
&value);
26622695
if (status != napi_ok) {
26632696
delete finalizeData;
26642697
NAPI_THROW_IF_FAILED(env, status, Buffer());
@@ -2681,7 +2714,8 @@ inline Buffer<T> Buffer<T>::New(napi_env env,
26812714
env,
26822715
length * sizeof(T),
26832716
data,
2684-
details::FinalizeData<T, Finalizer, Hint>::WrapperWithHint,
2717+
details::PostFinalizerWrapper<
2718+
details::FinalizeData<T, Finalizer, Hint>::WrapperWithHint>,
26852719
finalizeData,
26862720
&value);
26872721
if (status != napi_ok) {
@@ -2720,13 +2754,14 @@ inline Buffer<T> Buffer<T>::NewOrCopy(napi_env env,
27202754
{std::move(finalizeCallback), nullptr});
27212755
#ifndef NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED
27222756
napi_value value;
2723-
napi_status status =
2724-
napi_create_external_buffer(env,
2725-
length * sizeof(T),
2726-
data,
2727-
details::FinalizeData<T, Finalizer>::Wrapper,
2728-
finalizeData,
2729-
&value);
2757+
napi_status status = napi_create_external_buffer(
2758+
env,
2759+
length * sizeof(T),
2760+
data,
2761+
details::PostFinalizerWrapper<
2762+
details::FinalizeData<T, Finalizer>::Wrapper>,
2763+
finalizeData,
2764+
&value);
27302765
if (status == details::napi_no_external_buffers_allowed) {
27312766
#endif // NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED
27322767
// If we can't create an external buffer, we'll just copy the data.
@@ -2759,7 +2794,8 @@ inline Buffer<T> Buffer<T>::NewOrCopy(napi_env env,
27592794
env,
27602795
length * sizeof(T),
27612796
data,
2762-
details::FinalizeData<T, Finalizer, Hint>::WrapperWithHint,
2797+
details::PostFinalizerWrapper<
2798+
details::FinalizeData<T, Finalizer, Hint>::WrapperWithHint>,
27632799
finalizeData,
27642800
&value);
27652801
if (status == details::napi_no_external_buffers_allowed) {
@@ -3054,7 +3090,12 @@ inline void Error::ThrowAsJavaScriptException() const {
30543090

30553091
status = napi_throw(_env, Value());
30563092

3057-
if (status == napi_pending_exception) {
3093+
#ifdef NAPI_EXPERIMENTAL
3094+
napi_status expected_failure_mode = napi_cannot_run_js;
3095+
#else
3096+
napi_status expected_failure_mode = napi_pending_exception;
3097+
#endif
3098+
if (status == expected_failure_mode) {
30583099
// The environment must be terminating as we checked earlier and there
30593100
// was no pending exception. In this case continuing will result
30603101
// in a fatal error and there is nothing the author has done incorrectly
@@ -4428,7 +4469,12 @@ inline ObjectWrap<T>::ObjectWrap(const Napi::CallbackInfo& callbackInfo) {
44284469
napi_status status;
44294470
napi_ref ref;
44304471
T* instance = static_cast<T*>(this);
4431-
status = napi_wrap(env, wrapper, instance, FinalizeCallback, nullptr, &ref);
4472+
status = napi_wrap(env,
4473+
wrapper,
4474+
instance,
4475+
details::PostFinalizerWrapper<FinalizeCallback>,
4476+
nullptr,
4477+
&ref);
44324478
NAPI_THROW_IF_FAILED_VOID(env, status);
44334479

44344480
Reference<Object>* instanceRef = instance;
@@ -5324,19 +5370,21 @@ TypedThreadSafeFunction<ContextType, DataType, CallJs>::New(
53245370
auto* finalizeData = new details::
53255371
ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>(
53265372
{data, finalizeCallback});
5327-
napi_status status = napi_create_threadsafe_function(
5328-
env,
5329-
nullptr,
5330-
nullptr,
5331-
String::From(env, resourceName),
5332-
maxQueueSize,
5333-
initialThreadCount,
5334-
finalizeData,
5373+
auto fini =
53355374
details::ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>::
5336-
FinalizeFinalizeWrapperWithDataAndContext,
5337-
context,
5338-
CallJsInternal,
5339-
&tsfn._tsfn);
5375+
FinalizeFinalizeWrapperWithDataAndContext;
5376+
napi_status status =
5377+
napi_create_threadsafe_function(env,
5378+
nullptr,
5379+
nullptr,
5380+
String::From(env, resourceName),
5381+
maxQueueSize,
5382+
initialThreadCount,
5383+
finalizeData,
5384+
TSFN_FINALIZER(fini),
5385+
context,
5386+
CallJsInternal,
5387+
&tsfn._tsfn);
53405388
if (status != napi_ok) {
53415389
delete finalizeData;
53425390
NAPI_THROW_IF_FAILED(
@@ -5368,19 +5416,21 @@ TypedThreadSafeFunction<ContextType, DataType, CallJs>::New(
53685416
auto* finalizeData = new details::
53695417
ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>(
53705418
{data, finalizeCallback});
5371-
napi_status status = napi_create_threadsafe_function(
5372-
env,
5373-
nullptr,
5374-
resource,
5375-
String::From(env, resourceName),
5376-
maxQueueSize,
5377-
initialThreadCount,
5378-
finalizeData,
5419+
auto fini =
53795420
details::ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>::
5380-
FinalizeFinalizeWrapperWithDataAndContext,
5381-
context,
5382-
CallJsInternal,
5383-
&tsfn._tsfn);
5421+
FinalizeFinalizeWrapperWithDataAndContext;
5422+
napi_status status =
5423+
napi_create_threadsafe_function(env,
5424+
nullptr,
5425+
resource,
5426+
String::From(env, resourceName),
5427+
maxQueueSize,
5428+
initialThreadCount,
5429+
finalizeData,
5430+
TSFN_FINALIZER(fini),
5431+
context,
5432+
CallJsInternal,
5433+
&tsfn._tsfn);
53845434
if (status != napi_ok) {
53855435
delete finalizeData;
53865436
NAPI_THROW_IF_FAILED(
@@ -5484,19 +5534,21 @@ TypedThreadSafeFunction<ContextType, DataType, CallJs>::New(
54845534
auto* finalizeData = new details::
54855535
ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>(
54865536
{data, finalizeCallback});
5487-
napi_status status = napi_create_threadsafe_function(
5488-
env,
5489-
callback,
5490-
nullptr,
5491-
String::From(env, resourceName),
5492-
maxQueueSize,
5493-
initialThreadCount,
5494-
finalizeData,
5537+
auto fini =
54955538
details::ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>::
5496-
FinalizeFinalizeWrapperWithDataAndContext,
5497-
context,
5498-
CallJsInternal,
5499-
&tsfn._tsfn);
5539+
FinalizeFinalizeWrapperWithDataAndContext;
5540+
napi_status status =
5541+
napi_create_threadsafe_function(env,
5542+
callback,
5543+
nullptr,
5544+
String::From(env, resourceName),
5545+
maxQueueSize,
5546+
initialThreadCount,
5547+
finalizeData,
5548+
TSFN_FINALIZER(fini),
5549+
context,
5550+
CallJsInternal,
5551+
&tsfn._tsfn);
55005552
if (status != napi_ok) {
55015553
delete finalizeData;
55025554
NAPI_THROW_IF_FAILED(
@@ -5530,6 +5582,9 @@ TypedThreadSafeFunction<ContextType, DataType, CallJs>::New(
55305582
auto* finalizeData = new details::
55315583
ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>(
55325584
{data, finalizeCallback});
5585+
auto fini =
5586+
details::ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>::
5587+
FinalizeFinalizeWrapperWithDataAndContext;
55335588
napi_status status = napi_create_threadsafe_function(
55345589
env,
55355590
details::DefaultCallbackWrapper<
@@ -5541,8 +5596,7 @@ TypedThreadSafeFunction<ContextType, DataType, CallJs>::New(
55415596
maxQueueSize,
55425597
initialThreadCount,
55435598
finalizeData,
5544-
details::ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>::
5545-
FinalizeFinalizeWrapperWithDataAndContext,
5599+
TSFN_FINALIZER(fini),
55465600
context,
55475601
CallJsInternal,
55485602
&tsfn._tsfn);
@@ -6081,7 +6135,7 @@ inline ThreadSafeFunction ThreadSafeFunction::New(napi_env env,
60816135
maxQueueSize,
60826136
initialThreadCount,
60836137
finalizeData,
6084-
wrapper,
6138+
TSFN_FINALIZER(wrapper),
60856139
context,
60866140
CallJS,
60876141
&tsfn._tsfn);

‎test/threadsafe_function/threadsafe_function_existing_tsfn.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ static Value TestCall(const CallbackInfo& info) {
8686
0,
8787
1,
8888
nullptr, /*finalize data*/
89-
FinalizeCB,
89+
TSFN_FINALIZER(FinalizeCB),
9090
testContext,
9191
hasData ? CallJSWithData : CallJSNoData,
9292
&testContext->tsfn);

‎test/typed_threadsafe_function/typed_threadsafe_function_existing_tsfn.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ static Value TestCall(const CallbackInfo& info) {
8888
0,
8989
1,
9090
nullptr, /*finalize data*/
91-
FinalizeCB,
91+
TSFN_FINALIZER(FinalizeCB),
9292
testContext,
9393
hasData ? CallJSWithData : CallJSNoData,
9494
&testContext->tsfn);

0 commit comments

Comments
 (0)