Skip to content

Commit 08ce972

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 08ce972

File tree

5 files changed

+119
-64
lines changed

5 files changed

+119
-64
lines changed

‎.github/workflows/ci.yml

+11-1
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

+1
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

+105-61
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,23 @@ namespace details {
3131
// Node.js releases. Only necessary when they are used in napi.h and napi-inl.h.
3232
constexpr int napi_no_external_buffers_allowed = 22;
3333

34+
#if (defined(NAPI_EXPERIMENTAL) && \
35+
defined(NODE_API_EXPERIMENTAL_HAS_POST_FINALIZER))
36+
template <napi_finalize finalizer>
37+
inline void PostFinalizerWrapper(node_api_nogc_env nogc_env,
38+
void* data,
39+
void* hint) {
40+
napi_status status = node_api_post_finalizer(nogc_env, finalizer, data, hint);
41+
NAPI_FATAL_IF_FAILED(
42+
status, "PostFinalizerWrapper", "node_api_post_finalizer failed");
43+
}
44+
#else
45+
template <napi_finalize finalizer>
46+
inline void PostFinalizerWrapper(napi_env env, void* data, void* hint) {
47+
finalizer(env, data, hint);
48+
}
49+
#endif
50+
3451
template <typename FreeType>
3552
inline void default_finalizer(napi_env /*env*/, void* data, void* /*hint*/) {
3653
delete static_cast<FreeType*>(data);
@@ -65,7 +82,8 @@ inline napi_status AttachData(napi_env env,
6582
}
6683
}
6784
#else // NAPI_VERSION >= 5
68-
status = napi_add_finalizer(env, obj, data, finalizer, hint, nullptr);
85+
status = napi_add_finalizer(
86+
env, obj, data, details::PostFinalizerWrapper<finalizer>, hint, nullptr);
6987
#endif
7088
return status;
7189
}
@@ -1774,7 +1792,8 @@ inline External<T> External<T>::New(napi_env env,
17741792
napi_status status =
17751793
napi_create_external(env,
17761794
data,
1777-
details::FinalizeData<T, Finalizer>::Wrapper,
1795+
details::PostFinalizerWrapper<
1796+
details::FinalizeData<T, Finalizer>::Wrapper>,
17781797
finalizeData,
17791798
&value);
17801799
if (status != napi_ok) {
@@ -1797,7 +1816,8 @@ inline External<T> External<T>::New(napi_env env,
17971816
napi_status status = napi_create_external(
17981817
env,
17991818
data,
1800-
details::FinalizeData<T, Finalizer, Hint>::WrapperWithHint,
1819+
details::PostFinalizerWrapper<
1820+
details::FinalizeData<T, Finalizer, Hint>::WrapperWithHint>,
18011821
finalizeData,
18021822
&value);
18031823
if (status != napi_ok) {
@@ -1910,7 +1930,8 @@ inline ArrayBuffer ArrayBuffer::New(napi_env env,
19101930
env,
19111931
externalData,
19121932
byteLength,
1913-
details::FinalizeData<void, Finalizer>::Wrapper,
1933+
details::PostFinalizerWrapper<
1934+
details::FinalizeData<void, Finalizer>::Wrapper>,
19141935
finalizeData,
19151936
&value);
19161937
if (status != napi_ok) {
@@ -1935,7 +1956,8 @@ inline ArrayBuffer ArrayBuffer::New(napi_env env,
19351956
env,
19361957
externalData,
19371958
byteLength,
1938-
details::FinalizeData<void, Finalizer, Hint>::WrapperWithHint,
1959+
details::PostFinalizerWrapper<
1960+
details::FinalizeData<void, Finalizer, Hint>::WrapperWithHint>,
19391961
finalizeData,
19401962
&value);
19411963
if (status != napi_ok) {
@@ -2652,13 +2674,14 @@ inline Buffer<T> Buffer<T>::New(napi_env env,
26522674
details::FinalizeData<T, Finalizer>* finalizeData =
26532675
new details::FinalizeData<T, Finalizer>(
26542676
{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);
2677+
napi_status status = napi_create_external_buffer(
2678+
env,
2679+
length * sizeof(T),
2680+
data,
2681+
details::PostFinalizerWrapper<
2682+
details::FinalizeData<T, Finalizer>::Wrapper>,
2683+
finalizeData,
2684+
&value);
26622685
if (status != napi_ok) {
26632686
delete finalizeData;
26642687
NAPI_THROW_IF_FAILED(env, status, Buffer());
@@ -2681,7 +2704,8 @@ inline Buffer<T> Buffer<T>::New(napi_env env,
26812704
env,
26822705
length * sizeof(T),
26832706
data,
2684-
details::FinalizeData<T, Finalizer, Hint>::WrapperWithHint,
2707+
details::PostFinalizerWrapper<
2708+
details::FinalizeData<T, Finalizer, Hint>::WrapperWithHint>,
26852709
finalizeData,
26862710
&value);
26872711
if (status != napi_ok) {
@@ -2720,13 +2744,14 @@ inline Buffer<T> Buffer<T>::NewOrCopy(napi_env env,
27202744
{std::move(finalizeCallback), nullptr});
27212745
#ifndef NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED
27222746
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);
2747+
napi_status status = napi_create_external_buffer(
2748+
env,
2749+
length * sizeof(T),
2750+
data,
2751+
details::PostFinalizerWrapper<
2752+
details::FinalizeData<T, Finalizer>::Wrapper>,
2753+
finalizeData,
2754+
&value);
27302755
if (status == details::napi_no_external_buffers_allowed) {
27312756
#endif // NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED
27322757
// If we can't create an external buffer, we'll just copy the data.
@@ -2759,7 +2784,8 @@ inline Buffer<T> Buffer<T>::NewOrCopy(napi_env env,
27592784
env,
27602785
length * sizeof(T),
27612786
data,
2762-
details::FinalizeData<T, Finalizer, Hint>::WrapperWithHint,
2787+
details::PostFinalizerWrapper<
2788+
details::FinalizeData<T, Finalizer, Hint>::WrapperWithHint>,
27632789
finalizeData,
27642790
&value);
27652791
if (status == details::napi_no_external_buffers_allowed) {
@@ -3054,7 +3080,12 @@ inline void Error::ThrowAsJavaScriptException() const {
30543080

30553081
status = napi_throw(_env, Value());
30563082

3057-
if (status == napi_pending_exception) {
3083+
#ifdef NAPI_EXPERIMENTAL
3084+
napi_status expected_failure_mode = napi_cannot_run_js;
3085+
#else
3086+
napi_status expected_failure_mode = napi_pending_exception;
3087+
#endif
3088+
if (status == expected_failure_mode) {
30583089
// The environment must be terminating as we checked earlier and there
30593090
// was no pending exception. In this case continuing will result
30603091
// in a fatal error and there is nothing the author has done incorrectly
@@ -4428,7 +4459,12 @@ inline ObjectWrap<T>::ObjectWrap(const Napi::CallbackInfo& callbackInfo) {
44284459
napi_status status;
44294460
napi_ref ref;
44304461
T* instance = static_cast<T*>(this);
4431-
status = napi_wrap(env, wrapper, instance, FinalizeCallback, nullptr, &ref);
4462+
status = napi_wrap(env,
4463+
wrapper,
4464+
instance,
4465+
details::PostFinalizerWrapper<FinalizeCallback>,
4466+
nullptr,
4467+
&ref);
44324468
NAPI_THROW_IF_FAILED_VOID(env, status);
44334469

44344470
Reference<Object>* instanceRef = instance;
@@ -5324,19 +5360,21 @@ TypedThreadSafeFunction<ContextType, DataType, CallJs>::New(
53245360
auto* finalizeData = new details::
53255361
ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>(
53265362
{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,
5363+
auto fini =
53355364
details::ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>::
5336-
FinalizeFinalizeWrapperWithDataAndContext,
5337-
context,
5338-
CallJsInternal,
5339-
&tsfn._tsfn);
5365+
FinalizeFinalizeWrapperWithDataAndContext;
5366+
napi_status status =
5367+
napi_create_threadsafe_function(env,
5368+
nullptr,
5369+
nullptr,
5370+
String::From(env, resourceName),
5371+
maxQueueSize,
5372+
initialThreadCount,
5373+
finalizeData,
5374+
fini,
5375+
context,
5376+
CallJsInternal,
5377+
&tsfn._tsfn);
53405378
if (status != napi_ok) {
53415379
delete finalizeData;
53425380
NAPI_THROW_IF_FAILED(
@@ -5368,19 +5406,21 @@ TypedThreadSafeFunction<ContextType, DataType, CallJs>::New(
53685406
auto* finalizeData = new details::
53695407
ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>(
53705408
{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,
5409+
auto fini =
53795410
details::ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>::
5380-
FinalizeFinalizeWrapperWithDataAndContext,
5381-
context,
5382-
CallJsInternal,
5383-
&tsfn._tsfn);
5411+
FinalizeFinalizeWrapperWithDataAndContext;
5412+
napi_status status =
5413+
napi_create_threadsafe_function(env,
5414+
nullptr,
5415+
resource,
5416+
String::From(env, resourceName),
5417+
maxQueueSize,
5418+
initialThreadCount,
5419+
finalizeData,
5420+
fini,
5421+
context,
5422+
CallJsInternal,
5423+
&tsfn._tsfn);
53845424
if (status != napi_ok) {
53855425
delete finalizeData;
53865426
NAPI_THROW_IF_FAILED(
@@ -5484,19 +5524,21 @@ TypedThreadSafeFunction<ContextType, DataType, CallJs>::New(
54845524
auto* finalizeData = new details::
54855525
ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>(
54865526
{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,
5527+
auto fini =
54955528
details::ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>::
5496-
FinalizeFinalizeWrapperWithDataAndContext,
5497-
context,
5498-
CallJsInternal,
5499-
&tsfn._tsfn);
5529+
FinalizeFinalizeWrapperWithDataAndContext;
5530+
napi_status status =
5531+
napi_create_threadsafe_function(env,
5532+
callback,
5533+
nullptr,
5534+
String::From(env, resourceName),
5535+
maxQueueSize,
5536+
initialThreadCount,
5537+
finalizeData,
5538+
fini,
5539+
context,
5540+
CallJsInternal,
5541+
&tsfn._tsfn);
55005542
if (status != napi_ok) {
55015543
delete finalizeData;
55025544
NAPI_THROW_IF_FAILED(
@@ -5530,6 +5572,9 @@ TypedThreadSafeFunction<ContextType, DataType, CallJs>::New(
55305572
auto* finalizeData = new details::
55315573
ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>(
55325574
{data, finalizeCallback});
5575+
auto fini =
5576+
details::ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>::
5577+
FinalizeFinalizeWrapperWithDataAndContext;
55335578
napi_status status = napi_create_threadsafe_function(
55345579
env,
55355580
details::DefaultCallbackWrapper<
@@ -5541,8 +5586,7 @@ TypedThreadSafeFunction<ContextType, DataType, CallJs>::New(
55415586
maxQueueSize,
55425587
initialThreadCount,
55435588
finalizeData,
5544-
details::ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>::
5545-
FinalizeFinalizeWrapperWithDataAndContext,
5589+
fini,
55465590
context,
55475591
CallJsInternal,
55485592
&tsfn._tsfn);

‎test/threadsafe_function/threadsafe_function_existing_tsfn.cc

+1-1
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

+1-1
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)