Skip to content

Commit 486add4

Browse files
legendecaslegendecas
committed
node-api: generalize finalizer second pass callback
1 parent 9ec2787 commit 486add4

File tree

6 files changed

+198
-397
lines changed

6 files changed

+198
-397
lines changed

‎node.gyp‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -987,7 +987,6 @@
987987
'test/cctest/test_base_object_ptr.cc',
988988
'test/cctest/test_node_postmortem_metadata.cc',
989989
'test/cctest/test_environment.cc',
990-
'test/cctest/test_js_native_api_v8.cc',
991990
'test/cctest/test_linked_binding.cc',
992991
'test/cctest/test_node_api.cc',
993992
'test/cctest/test_per_process.cc',

‎src/js_native_api_v8.cc‎

Lines changed: 80 additions & 162 deletions
Original file line numberDiff line numberDiff line change
@@ -191,10 +191,10 @@ inline static napi_status ConcludeDeferred(napi_env env,
191191

192192
enum UnwrapAction { KeepWrap, RemoveWrap };
193193

194-
inline static napi_status Unwrap(napi_env env,
195-
napi_value js_object,
196-
void** result,
197-
UnwrapAction action) {
194+
inline napi_status Unwrap(napi_env env,
195+
napi_value js_object,
196+
void** result,
197+
UnwrapAction action) {
198198
NAPI_PREAMBLE(env);
199199
CHECK_ARG(env, js_object);
200200
if (action == KeepWrap) {
@@ -220,7 +220,7 @@ inline static napi_status Unwrap(napi_env env,
220220
if (action == RemoveWrap) {
221221
CHECK(obj->DeletePrivate(context, NAPI_PRIVATE_KEY(context, wrapper))
222222
.FromJust());
223-
Reference::Delete(reference);
223+
delete reference;
224224
}
225225

226226
return GET_RETURN_STATUS(env);
@@ -466,33 +466,12 @@ RefBase::RefBase(napi_env env,
466466
void* finalize_data,
467467
void* finalize_hint)
468468
: Finalizer(env, finalize_callback, finalize_data, finalize_hint),
469-
_refcount(initial_refcount),
470-
_delete_self(delete_self) {
469+
refcount_(initial_refcount),
470+
delete_self_(delete_self) {
471471
Link(finalize_callback == nullptr ? &env->reflist : &env->finalizing_reflist);
472472
}
473473

474-
RefBase* RefBase::New(napi_env env,
475-
uint32_t initial_refcount,
476-
bool delete_self,
477-
napi_finalize finalize_callback,
478-
void* finalize_data,
479-
void* finalize_hint) {
480-
return new RefBase(env,
481-
initial_refcount,
482-
delete_self,
483-
finalize_callback,
484-
finalize_data,
485-
finalize_hint);
486-
}
487-
488-
RefBase::~RefBase() {
489-
Unlink();
490-
}
491-
492-
void* RefBase::Data() {
493-
return _finalize_data;
494-
}
495-
474+
// TODO(@legendecas): document this better.
496475
// Delete is called in 2 ways. Either from the finalizer or
497476
// from one of Unwrap or napi_delete_reference.
498477
//
@@ -508,82 +487,82 @@ void* RefBase::Data() {
508487
// The second way this is called is from
509488
// the finalizer and _delete_self is set. In this case we
510489
// know we need to do the deletion so just do it.
511-
void RefBase::Delete(RefBase* reference) {
512-
if ((reference->RefCount() != 0) || (reference->_delete_self) ||
513-
(reference->_finalize_ran)) {
514-
delete reference;
515-
} else {
516-
// defer until finalizer runs as
517-
// it may already be queued
518-
reference->_delete_self = true;
519-
}
490+
RefBase::~RefBase() {
491+
Unlink();
492+
// Try to remove the finalizer from the scheduled second pass callback.
493+
env_->DequeueFinalizer(this);
494+
}
495+
496+
RefBase* RefBase::New(napi_env env,
497+
uint32_t initial_refcount,
498+
bool delete_self,
499+
napi_finalize finalize_callback,
500+
void* finalize_data,
501+
void* finalize_hint) {
502+
return new RefBase(env,
503+
initial_refcount,
504+
delete_self,
505+
finalize_callback,
506+
finalize_data,
507+
finalize_hint);
508+
}
509+
510+
void* RefBase::Data() {
511+
return finalize_data_;
520512
}
521513

522514
uint32_t RefBase::Ref() {
523-
return ++_refcount;
515+
return ++refcount_;
524516
}
525517

526518
uint32_t RefBase::Unref() {
527-
if (_refcount == 0) {
519+
if (refcount_ == 0) {
528520
return 0;
529521
}
530-
return --_refcount;
522+
return --refcount_;
531523
}
532524

533525
uint32_t RefBase::RefCount() {
534-
return _refcount;
535-
}
536-
537-
void RefBase::Finalize(bool is_env_teardown) {
538-
// In addition to being called during environment teardown, this method is
539-
// also the entry point for the garbage collector. During environment
540-
// teardown we have to remove the garbage collector's reference to this
541-
// method so that, if, as part of the user's callback, JS gets executed,
542-
// resulting in a garbage collection pass, this method is not re-entered as
543-
// part of that pass, because that'll cause a double free (as seen in
544-
// https://github.com/nodejs/node/issues/37236).
545-
//
546-
// Since this class does not have access to the V8 persistent reference,
547-
// this method is overridden in the `Reference` class below. Therein the
548-
// weak callback is removed, ensuring that the garbage collector does not
549-
// re-enter this method, and the method chains up to continue the process of
550-
// environment-teardown-induced finalization.
551-
552-
// During environment teardown we have to convert a strong reference to
553-
// a weak reference to force the deferring behavior if the user's finalizer
554-
// happens to delete this reference so that the code in this function that
555-
// follows the call to the user's finalizer may safely access variables from
556-
// this instance.
557-
if (is_env_teardown && RefCount() > 0) _refcount = 0;
558-
559-
if (_finalize_callback != nullptr) {
560-
// This ensures that we never call the finalizer twice.
561-
napi_finalize fini = _finalize_callback;
562-
_finalize_callback = nullptr;
563-
_env->CallFinalizer(fini, _finalize_data, _finalize_hint);
564-
}
565-
566-
// this is safe because if a request to delete the reference
567-
// is made in the finalize_callback it will defer deletion
568-
// to this block and set _delete_self to true
569-
if (_delete_self || is_env_teardown) {
570-
Delete(this);
571-
} else {
572-
_finalize_ran = true;
526+
return refcount_;
527+
}
528+
529+
void RefBase::Finalize() {
530+
napi_finalize finalize_callback = finalize_callback_;
531+
bool delete_self = delete_self_;
532+
533+
// Either RefBase is deleted or not, it should be removed from the tracked
534+
// list.
535+
Unlink();
536+
// 1. If the finalizer is present, it should either delete the RefBase in the
537+
// finalizer, or set the flag `delete_self`.
538+
// 2. If the finalizer is not present, the RefBase can be deleted after the
539+
// call.
540+
if (finalize_callback != nullptr) {
541+
env_->CallFinalizer(finalize_callback, finalize_data_, finalize_hint_);
542+
// No access to this after Finalizer is called.
543+
}
544+
545+
// If the RefBase is not self-destructive, it should have been deleted in the
546+
// finalizer. Now delete it if it is self-destructive.
547+
if (delete_self) {
548+
delete this;
573549
}
574550
}
575551

576552
template <typename... Args>
577553
Reference::Reference(napi_env env, v8::Local<v8::Value> value, Args&&... args)
578554
: RefBase(env, std::forward<Args>(args)...),
579-
_persistent(env->isolate, value),
580-
_secondPassParameter(new SecondPassCallParameterRef(this)),
581-
_secondPassScheduled(false) {
555+
persistent_(env->isolate, value) {
582556
if (RefCount() == 0) {
583557
SetWeak();
584558
}
585559
}
586560

561+
Reference::~Reference() {
562+
// Reset the handle. And no weak callback will be invoked.
563+
persistent_.Reset();
564+
}
565+
587566
Reference* Reference::New(napi_env env,
588567
v8::Local<v8::Value> value,
589568
uint32_t initial_refcount,
@@ -600,15 +579,6 @@ Reference* Reference::New(napi_env env,
600579
finalize_hint);
601580
}
602581

603-
Reference::~Reference() {
604-
// If the second pass callback is scheduled, it will delete the
605-
// parameter passed to it, otherwise it will never be scheduled
606-
// and we need to delete it here.
607-
if (!_secondPassScheduled) {
608-
delete _secondPassParameter;
609-
}
610-
}
611-
612582
uint32_t Reference::Ref() {
613583
uint32_t refcount = RefBase::Ref();
614584
if (refcount == 1) {
@@ -627,25 +597,20 @@ uint32_t Reference::Unref() {
627597
}
628598

629599
v8::Local<v8::Value> Reference::Get() {
630-
if (_persistent.IsEmpty()) {
600+
if (persistent_.IsEmpty()) {
631601
return v8::Local<v8::Value>();
632602
} else {
633-
return v8::Local<v8::Value>::New(_env->isolate, _persistent);
603+
return v8::Local<v8::Value>::New(env_->isolate, persistent_);
634604
}
635605
}
636606

637-
void Reference::Finalize(bool is_env_teardown) {
638-
// During env teardown, `~napi_env()` alone is responsible for finalizing.
639-
// Thus, we don't want any stray gc passes to trigger a second call to
640-
// `RefBase::Finalize()`. ClearWeak will ensure that even if the
641-
// gc is in progress no Finalization will be run for this Reference
642-
// by the gc.
643-
if (is_env_teardown) {
644-
ClearWeak();
645-
}
607+
void Reference::Finalize() {
608+
// Unconditionally reset the persistent handle so that no weak callback will
609+
// be invoked again.
610+
persistent_.Reset();
646611

647612
// Chain up to perform the rest of the finalization.
648-
RefBase::Finalize(is_env_teardown);
613+
RefBase::Finalize();
649614
}
650615

651616
// ClearWeak is marking the Reference so that the gc should not
@@ -654,72 +619,25 @@ void Reference::Finalize(bool is_env_teardown) {
654619
// the secondPassParameter so that even if it has been
655620
// scheduled no Finalization will be run.
656621
void Reference::ClearWeak() {
657-
if (!_persistent.IsEmpty()) {
658-
_persistent.ClearWeak();
659-
}
660-
if (_secondPassParameter != nullptr) {
661-
*_secondPassParameter = nullptr;
622+
if (!persistent_.IsEmpty()) {
623+
persistent_.ClearWeak();
662624
}
663625
}
664626

665627
// Mark the reference as weak and eligible for collection
666628
// by the gc.
667629
void Reference::SetWeak() {
668-
if (_secondPassParameter == nullptr) {
669-
// This means that the Reference has already been processed
670-
// by the second pass callback, so its already been Finalized, do
671-
// nothing
672-
return;
673-
}
674-
_persistent.SetWeak(
675-
_secondPassParameter, FinalizeCallback, v8::WeakCallbackType::kParameter);
676-
*_secondPassParameter = this;
630+
persistent_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter);
677631
}
678632

679633
// The N-API finalizer callback may make calls into the engine. V8's heap is
680634
// not in a consistent state during the weak callback, and therefore it does
681-
// not support calls back into it. However, it provides a mechanism for adding
682-
// a finalizer which may make calls back into the engine by allowing us to
683-
// attach such a second-pass finalizer from the first pass finalizer. Thus,
684-
// we do that here to ensure that the N-API finalizer callback is free to call
685-
// into the engine.
686-
void Reference::FinalizeCallback(
687-
const v8::WeakCallbackInfo<SecondPassCallParameterRef>& data) {
688-
SecondPassCallParameterRef* parameter = data.GetParameter();
689-
Reference* reference = *parameter;
690-
if (reference == nullptr) {
691-
return;
692-
}
693-
694-
// The reference must be reset during the first pass.
695-
reference->_persistent.Reset();
696-
// Mark the parameter not delete-able until the second pass callback is
697-
// invoked.
698-
reference->_secondPassScheduled = true;
699-
700-
data.SetSecondPassCallback(SecondPassCallback);
701-
}
702-
703-
// Second pass callbacks are scheduled with platform tasks. At env teardown,
704-
// the tasks may have already be scheduled and we are unable to cancel the
705-
// second pass callback task. We have to make sure that parameter is kept
706-
// alive until the second pass callback is been invoked. In order to do
707-
// this and still allow our code to Finalize/delete the Reference during
708-
// shutdown we have to use a separately allocated parameter instead
709-
// of a parameter within the Reference object itself. This is what
710-
// is stored in _secondPassParameter and it is allocated in the
711-
// constructor for the Reference.
712-
void Reference::SecondPassCallback(
713-
const v8::WeakCallbackInfo<SecondPassCallParameterRef>& data) {
714-
SecondPassCallParameterRef* parameter = data.GetParameter();
715-
Reference* reference = *parameter;
716-
delete parameter;
717-
if (reference == nullptr) {
718-
// the reference itself has already been deleted so nothing to do
719-
return;
720-
}
721-
reference->_secondPassParameter = nullptr;
722-
reference->Finalize();
635+
// not support calls back into it. Enqueue the invocation of the finalizer.
636+
void Reference::WeakCallback(const v8::WeakCallbackInfo<Reference>& data) {
637+
Reference* reference = data.GetParameter();
638+
// The reference must be reset during the weak callback as the API protocol.
639+
reference->persistent_.Reset();
640+
reference->env_->EnqueueFinalizer(reference);
723641
}
724642

725643
} // end of namespace v8impl
@@ -2515,7 +2433,7 @@ napi_status NAPI_CDECL napi_delete_reference(napi_env env, napi_ref ref) {
25152433
CHECK_ENV(env);
25162434
CHECK_ARG(env, ref);
25172435

2518-
v8impl::Reference::Delete(reinterpret_cast<v8impl::Reference*>(ref));
2436+
delete reinterpret_cast<v8impl::Reference*>(ref);
25192437

25202438
return napi_clear_last_error(env);
25212439
}
@@ -3205,7 +3123,7 @@ napi_status NAPI_CDECL napi_set_instance_data(napi_env env,
32053123
if (old_data != nullptr) {
32063124
// Our contract so far has been to not finalize any old data there may be.
32073125
// So we simply delete it.
3208-
v8impl::RefBase::Delete(old_data);
3126+
delete old_data;
32093127
}
32103128

32113129
env->instance_data =

0 commit comments

Comments
 (0)