Skip to content

Commit

Permalink
src: restore ability to run under NAPI_EXPERIMENTAL
Browse files Browse the repository at this point in the history 
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`.
  • Loading branch information
@gabrielschulhof
gabrielschulhof committed Feb 18, 2024
1 parent f85e814 commit f89a09b
Show file tree
Hide file tree
Showing 5 changed files with 125 additions and 64 deletions.
6 changes: 6 additions & 0 deletions .github/workflows/ci.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ jobs:
timeout-minutes: 30
strategy:
matrix:
api_version:
- standard
- experimental
node-version: [ 18.x, 20.x, 21.x ]
os:
- macos-latest
Expand Down Expand Up @@ -43,6 +46,9 @@ jobs:
npm install
- name: npm test
run: |
if [ "${{ matrix.api_version }}" = "experimental" ]; then
export NAPI_VERSION=2147483647
fi
if [ "${{ matrix.compiler }}" = "gcc" ]; then
export CC="gcc" CXX="g++"
fi
Expand Down
1 change: 1 addition & 0 deletions common.gypi
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
},
'conditions': [
['NAPI_VERSION!=""', { 'defines': ['NAPI_VERSION=<@(NAPI_VERSION)'] } ],
['NAPI_VERSION==2147483647', { 'defines': ['NAPI_EXPERIMENTAL'] } ],
['disable_deprecated=="true"', {
'defines': ['NODE_ADDON_API_DISABLE_DEPRECATED']
}],
Expand Down
178 changes: 116 additions & 62 deletions napi-inl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,16 @@
#include <type_traits>
#include <utility>

// TODO(gabrielschulhof): Remove this and remove the wrapping at the call sites
// (i.e., at the call site, `TSFN_FINALIZER(x)` should be changed back to `x`)
// after https://github.com/nodejs/node/pull/51801 has landed.
#if (defined(NAPI_EXPERIMENTAL) && \
defined(NODE_API_EXPERIMENTAL_HAS_POST_FINALIZER))
#define TSFN_FINALIZER(fini) reinterpret_cast<node_api_nogc_finalize>(fini)
#else
#define TSFN_FINALIZER(fini) fini
#endif

namespace Napi {

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

#if (defined(NAPI_EXPERIMENTAL) && \
defined(NODE_API_EXPERIMENTAL_HAS_POST_FINALIZER))
template <napi_finalize finalizer>
inline void PostFinalizerWrapper(node_api_nogc_env nogc_env,
void* data,
void* hint) {
napi_status status = node_api_post_finalizer(nogc_env, finalizer, data, hint);
NAPI_FATAL_IF_FAILED(
status, "PostFinalizerWrapper", "node_api_post_finalizer failed");
}
#else
template <napi_finalize finalizer>
inline void PostFinalizerWrapper(napi_env env, void* data, void* hint) {
finalizer(env, data, hint);
}
#endif

template <typename FreeType>
inline void default_finalizer(napi_env /*env*/, void* data, void* /*hint*/) {
delete static_cast<FreeType*>(data);
Expand Down Expand Up @@ -65,7 +92,8 @@ inline napi_status AttachData(napi_env env,
}
}
#else // NAPI_VERSION >= 5
status = napi_add_finalizer(env, obj, data, finalizer, hint, nullptr);
status = napi_add_finalizer(
env, obj, data, details::PostFinalizerWrapper<finalizer>, hint, nullptr);
#endif
return status;
}
Expand Down Expand Up @@ -1774,7 +1802,8 @@ inline External<T> External<T>::New(napi_env env,
napi_status status =
napi_create_external(env,
data,
details::FinalizeData<T, Finalizer>::Wrapper,
details::PostFinalizerWrapper<
details::FinalizeData<T, Finalizer>::Wrapper>,
finalizeData,
&value);
if (status != napi_ok) {
Expand All @@ -1797,7 +1826,8 @@ inline External<T> External<T>::New(napi_env env,
napi_status status = napi_create_external(
env,
data,
details::FinalizeData<T, Finalizer, Hint>::WrapperWithHint,
details::PostFinalizerWrapper<
details::FinalizeData<T, Finalizer, Hint>::WrapperWithHint>,
finalizeData,
&value);
if (status != napi_ok) {
Expand Down Expand Up @@ -1910,7 +1940,8 @@ inline ArrayBuffer ArrayBuffer::New(napi_env env,
env,
externalData,
byteLength,
details::FinalizeData<void, Finalizer>::Wrapper,
details::PostFinalizerWrapper<
details::FinalizeData<void, Finalizer>::Wrapper>,
finalizeData,
&value);
if (status != napi_ok) {
Expand All @@ -1935,7 +1966,8 @@ inline ArrayBuffer ArrayBuffer::New(napi_env env,
env,
externalData,
byteLength,
details::FinalizeData<void, Finalizer, Hint>::WrapperWithHint,
details::PostFinalizerWrapper<
details::FinalizeData<void, Finalizer, Hint>::WrapperWithHint>,
finalizeData,
&value);
if (status != napi_ok) {
Expand Down Expand Up @@ -2652,13 +2684,14 @@ inline Buffer<T> Buffer<T>::New(napi_env env,
details::FinalizeData<T, Finalizer>* finalizeData =
new details::FinalizeData<T, Finalizer>(
{std::move(finalizeCallback), nullptr});
napi_status status =
napi_create_external_buffer(env,
length * sizeof(T),
data,
details::FinalizeData<T, Finalizer>::Wrapper,
finalizeData,
&value);
napi_status status = napi_create_external_buffer(
env,
length * sizeof(T),
data,
details::PostFinalizerWrapper<
details::FinalizeData<T, Finalizer>::Wrapper>,
finalizeData,
&value);
if (status != napi_ok) {
delete finalizeData;
NAPI_THROW_IF_FAILED(env, status, Buffer());
Expand All @@ -2681,7 +2714,8 @@ inline Buffer<T> Buffer<T>::New(napi_env env,
env,
length * sizeof(T),
data,
details::FinalizeData<T, Finalizer, Hint>::WrapperWithHint,
details::PostFinalizerWrapper<
details::FinalizeData<T, Finalizer, Hint>::WrapperWithHint>,
finalizeData,
&value);
if (status != napi_ok) {
Expand Down Expand Up @@ -2720,13 +2754,14 @@ inline Buffer<T> Buffer<T>::NewOrCopy(napi_env env,
{std::move(finalizeCallback), nullptr});
#ifndef NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED
napi_value value;
napi_status status =
napi_create_external_buffer(env,
length * sizeof(T),
data,
details::FinalizeData<T, Finalizer>::Wrapper,
finalizeData,
&value);
napi_status status = napi_create_external_buffer(
env,
length * sizeof(T),
data,
details::PostFinalizerWrapper<
details::FinalizeData<T, Finalizer>::Wrapper>,
finalizeData,
&value);
if (status == details::napi_no_external_buffers_allowed) {
#endif // NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED
// If we can't create an external buffer, we'll just copy the data.
Expand Down Expand Up @@ -2759,7 +2794,8 @@ inline Buffer<T> Buffer<T>::NewOrCopy(napi_env env,
env,
length * sizeof(T),
data,
details::FinalizeData<T, Finalizer, Hint>::WrapperWithHint,
details::PostFinalizerWrapper<
details::FinalizeData<T, Finalizer, Hint>::WrapperWithHint>,
finalizeData,
&value);
if (status == details::napi_no_external_buffers_allowed) {
Expand Down Expand Up @@ -3054,7 +3090,12 @@ inline void Error::ThrowAsJavaScriptException() const {

status = napi_throw(_env, Value());

if (status == napi_pending_exception) {
#ifdef NAPI_EXPERIMENTAL
napi_status expected_failure_mode = napi_cannot_run_js;
#else
napi_status expected_failure_mode = napi_pending_exception;
#endif
if (status == expected_failure_mode) {
// The environment must be terminating as we checked earlier and there
// was no pending exception. In this case continuing will result
// in a fatal error and there is nothing the author has done incorrectly
Expand Down Expand Up @@ -4428,7 +4469,12 @@ inline ObjectWrap<T>::ObjectWrap(const Napi::CallbackInfo& callbackInfo) {
napi_status status;
napi_ref ref;
T* instance = static_cast<T*>(this);
status = napi_wrap(env, wrapper, instance, FinalizeCallback, nullptr, &ref);
status = napi_wrap(env,
wrapper,
instance,
details::PostFinalizerWrapper<FinalizeCallback>,
nullptr,
&ref);
NAPI_THROW_IF_FAILED_VOID(env, status);

Reference<Object>* instanceRef = instance;
Expand Down Expand Up @@ -5321,19 +5367,21 @@ TypedThreadSafeFunction<ContextType, DataType, CallJs>::New(
auto* finalizeData = new details::
ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>(
{data, finalizeCallback});
napi_status status = napi_create_threadsafe_function(
env,
nullptr,
nullptr,
String::From(env, resourceName),
maxQueueSize,
initialThreadCount,
finalizeData,
auto fini =
details::ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>::
FinalizeFinalizeWrapperWithDataAndContext,
context,
CallJsInternal,
&tsfn._tsfn);
FinalizeFinalizeWrapperWithDataAndContext;
napi_status status =
napi_create_threadsafe_function(env,
nullptr,
nullptr,
String::From(env, resourceName),
maxQueueSize,
initialThreadCount,
finalizeData,
TSFN_FINALIZER(fini),
context,
CallJsInternal,
&tsfn._tsfn);
if (status != napi_ok) {
delete finalizeData;
NAPI_THROW_IF_FAILED(
Expand Down Expand Up @@ -5365,19 +5413,21 @@ TypedThreadSafeFunction<ContextType, DataType, CallJs>::New(
auto* finalizeData = new details::
ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>(
{data, finalizeCallback});
napi_status status = napi_create_threadsafe_function(
env,
nullptr,
resource,
String::From(env, resourceName),
maxQueueSize,
initialThreadCount,
finalizeData,
auto fini =
details::ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>::
FinalizeFinalizeWrapperWithDataAndContext,
context,
CallJsInternal,
&tsfn._tsfn);
FinalizeFinalizeWrapperWithDataAndContext;
napi_status status =
napi_create_threadsafe_function(env,
nullptr,
resource,
String::From(env, resourceName),
maxQueueSize,
initialThreadCount,
finalizeData,
TSFN_FINALIZER(fini),
context,
CallJsInternal,
&tsfn._tsfn);
if (status != napi_ok) {
delete finalizeData;
NAPI_THROW_IF_FAILED(
Expand Down Expand Up @@ -5481,19 +5531,21 @@ TypedThreadSafeFunction<ContextType, DataType, CallJs>::New(
auto* finalizeData = new details::
ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>(
{data, finalizeCallback});
napi_status status = napi_create_threadsafe_function(
env,
callback,
nullptr,
String::From(env, resourceName),
maxQueueSize,
initialThreadCount,
finalizeData,
auto fini =
details::ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>::
FinalizeFinalizeWrapperWithDataAndContext,
context,
CallJsInternal,
&tsfn._tsfn);
FinalizeFinalizeWrapperWithDataAndContext;
napi_status status =
napi_create_threadsafe_function(env,
callback,
nullptr,
String::From(env, resourceName),
maxQueueSize,
initialThreadCount,
finalizeData,
TSFN_FINALIZER(fini),
context,
CallJsInternal,
&tsfn._tsfn);
if (status != napi_ok) {
delete finalizeData;
NAPI_THROW_IF_FAILED(
Expand Down Expand Up @@ -5527,6 +5579,9 @@ TypedThreadSafeFunction<ContextType, DataType, CallJs>::New(
auto* finalizeData = new details::
ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>(
{data, finalizeCallback});
auto fini =
details::ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>::
FinalizeFinalizeWrapperWithDataAndContext;
napi_status status = napi_create_threadsafe_function(
env,
details::DefaultCallbackWrapper<
Expand All @@ -5538,8 +5593,7 @@ TypedThreadSafeFunction<ContextType, DataType, CallJs>::New(
maxQueueSize,
initialThreadCount,
finalizeData,
details::ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>::
FinalizeFinalizeWrapperWithDataAndContext,
TSFN_FINALIZER(fini),
context,
CallJsInternal,
&tsfn._tsfn);
Expand Down Expand Up @@ -6078,7 +6132,7 @@ inline ThreadSafeFunction ThreadSafeFunction::New(napi_env env,
maxQueueSize,
initialThreadCount,
finalizeData,
wrapper,
TSFN_FINALIZER(wrapper),
context,
CallJS,
&tsfn._tsfn);
Expand Down
2 changes: 1 addition & 1 deletion test/threadsafe_function/threadsafe_function_existing_tsfn.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ static Value TestCall(const CallbackInfo& info) {
0,
1,
nullptr, /*finalize data*/
FinalizeCB,
TSFN_FINALIZER(FinalizeCB),
testContext,
hasData ? CallJSWithData : CallJSNoData,
&testContext->tsfn);
Expand Down
2 changes: 1 addition & 1 deletion test/typed_threadsafe_function/typed_threadsafe_function_existing_tsfn.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ static Value TestCall(const CallbackInfo& info) {
0,
1,
nullptr, /*finalize data*/
FinalizeCB,
TSFN_FINALIZER(FinalizeCB),
testContext,
hasData ? CallJSWithData : CallJSNoData,
&testContext->tsfn);
Expand Down

0 comments on commit f89a09b

Please sign in to comment.