Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
n-api: revert change to finalization
Fixes: #35620

This reverts commit a6b6556 which
changed finalization behavior related to N-API. We will investigate
the original issue with the test separately.

PR-URL: #35777
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
  • Loading branch information
mhdawson authored and MylesBorins committed Nov 16, 2020
1 parent df52814 commit a93ca2d
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 4 deletions.
6 changes: 2 additions & 4 deletions src/js_native_api_v8.cc
Expand Up @@ -228,10 +228,9 @@ class RefBase : protected Finalizer, RefTracker {
// from one of Unwrap or napi_delete_reference.
//
// When it is called from Unwrap or napi_delete_reference we only
// want to do the delete if there is no finalizer or the finalizer has already
// run or cannot have been queued to run (i.e. the reference count is > 0),
// want to do the delete if the finalizer has already run or
// cannot have been queued to run (ie the reference count is > 0),
// otherwise we may crash when the finalizer does run.
//
// If the finalizer may have been queued and has not already run
// delay the delete until the finalizer runs by not doing the delete
// and setting _delete_self to true so that the finalizer will
Expand All @@ -243,7 +242,6 @@ class RefBase : protected Finalizer, RefTracker {
static inline void Delete(RefBase* reference) {
reference->Unlink();
if ((reference->RefCount() != 0) ||
(reference->_finalize_callback == nullptr) ||
(reference->_delete_self) ||
(reference->_finalize_ran)) {
delete reference;
Expand Down
6 changes: 6 additions & 0 deletions test/node-api/test_worker_terminate_finalization/test.js
@@ -1,6 +1,12 @@
'use strict';
const common = require('../../common');

// TODO(addaleax): Run this test once it stops failing under ASAN/valgrind.
// Refs: https://github.com/nodejs/node/issues/34731
// Refs: https://github.com/nodejs/node/pull/35777
// Refs: https://github.com/nodejs/node/issues/35778
common.skip('Reference management in N-API leaks memory');

const { Worker, isMainThread } = require('worker_threads');

if (isMainThread) {
Expand Down

0 comments on commit a93ca2d

Please sign in to comment.