GeezerVison is showing github.com — node-api: stop ref gc during environment teardown · nodejs/node@6e01855
File treeExpand file treeCollapse file tree Open diff view settings test/node-api/test_env_teardown_gc
Expand file treeCollapse file tree Open diff view settings Collapse file Copy file name to clipboardExpand all lines: src/js_native_api_v8.cc +27-1Lines changed: 27 additions & 1 deletion | Original file line number | Diff line number | Diff line change |
|---|
@@ -270,6 +270,20 @@ class RefBase : protected Finalizer, RefTracker { | 270 | 270 |
| 271 | 271 | protected: | 272 | 272 | inline void Finalize(bool is_env_teardown = false) override { | | 273 | + // In addition to being called during environment teardown, this method is | | 274 | + // also the entry point for the garbage collector. During environment | | 275 | + // teardown we have to remove the garbage collector's reference to this | | 276 | + // method so that, if, as part of the user's callback, JS gets executed, | | 277 | + // resulting in a garbage collection pass, this method is not re-entered as | | 278 | + // part of that pass, because that'll cause a double free (as seen in | | 279 | + // https://github.com/nodejs/node/issues/37236). | | 280 | + // | | 281 | + // Since this class does not have access to the V8 persistent reference, | | 282 | + // this method is overridden in the `Reference` class below. Therein the | | 283 | + // weak callback is removed, ensuring that the garbage collector does not | | 284 | + // re-enter this method, and the method chains up to continue the process of | | 285 | + // environment-teardown-induced finalization. | | 286 | + | 273 | 287 | // During environment teardown we have to convert a strong reference to | 274 | 288 | // a weak reference to force the deferring behavior if the user's finalizer | 275 | 289 | // happens to delete this reference so that the code in this function that | @@ -278,9 +292,10 @@ class RefBase : protected Finalizer, RefTracker { | 278 | 292 | if (is_env_teardown && RefCount() > 0) _refcount = 0; | 279 | 293 |
| 280 | 294 | if (_finalize_callback != nullptr) { | 281 | | - _env->CallFinalizer(_finalize_callback, _finalize_data, _finalize_hint); | 282 | 295 | // This ensures that we never call the finalizer twice. | | 296 | + napi_finalize fini = _finalize_callback; | 283 | 297 | _finalize_callback = nullptr; | | 298 | + _env->CallFinalizer(fini, _finalize_data, _finalize_hint); | 284 | 299 | } | 285 | 300 |
| 286 | 301 | // this is safe because if a request to delete the reference | @@ -355,6 +370,17 @@ class Reference : public RefBase { | 355 | 370 | } | 356 | 371 | } | 357 | 372 |
| | 373 | + protected: | | 374 | + inline void Finalize(bool is_env_teardown = false) override { | | 375 | + // During env teardown, `~napi_env()` alone is responsible for finalizing. | | 376 | + // Thus, we don't want any stray gc passes to trigger a second call to | | 377 | + // `Finalize()`, so let's reset the persistent here. | | 378 | + if (is_env_teardown) _persistent.ClearWeak(); | | 379 | + | | 380 | + // Chain up to perform the rest of the finalization. | | 381 | + RefBase::Finalize(is_env_teardown); | | 382 | + } | | 383 | + | 358 | 384 | private: | 359 | 385 | // The N-API finalizer callback may make calls into the engine. V8's heap is | 360 | 386 | // not in a consistent state during the weak callback, and therefore it does | |
Collapse file Copy file name to clipboard +37Lines changed: 37 additions & 0 deletions | Original file line number | Diff line number | Diff line change |
|---|
| | 1 | +#include <stdlib.h> | | 2 | +#include <node_api.h> | | 3 | +#include "../../js-native-api/common.h" | | 4 | + | | 5 | +static void MyObject_fini(napi_env env, void* data, void* hint) { | | 6 | + napi_ref* ref = data; | | 7 | + napi_value global; | | 8 | + napi_value cleanup; | | 9 | + NAPI_CALL_RETURN_VOID(env, napi_get_global(env, &global)); | | 10 | + NAPI_CALL_RETURN_VOID( | | 11 | + env, napi_get_named_property(env, global, "cleanup", &cleanup)); | | 12 | + napi_status status = napi_call_function(env, global, cleanup, 0, NULL, NULL); | | 13 | + // We may not be allowed to call into JS, in which case a pending exception | | 14 | + // will be returned. | | 15 | + NAPI_ASSERT_RETURN_VOID(env, | | 16 | + status == napi_ok || status == napi_pending_exception, | | 17 | + "Unexpected status for napi_call_function"); | | 18 | + NAPI_CALL_RETURN_VOID(env, napi_delete_reference(env, *ref)); | | 19 | + free(ref); | | 20 | +} | | 21 | + | | 22 | +static napi_value MyObject(napi_env env, napi_callback_info info) { | | 23 | + napi_value js_this; | | 24 | + napi_ref* ref = malloc(sizeof(*ref)); | | 25 | + NAPI_CALL(env, napi_get_cb_info(env, info, NULL, NULL, &js_this, NULL)); | | 26 | + NAPI_CALL(env, napi_wrap(env, js_this, ref, MyObject_fini, NULL, ref)); | | 27 | + return NULL; | | 28 | +} | | 29 | + | | 30 | +NAPI_MODULE_INIT() { | | 31 | + napi_value ctor; | | 32 | + NAPI_CALL( | | 33 | + env, napi_define_class( | | 34 | + env, "MyObject", NAPI_AUTO_LENGTH, MyObject, NULL, 0, NULL, &ctor)); | | 35 | + NAPI_CALL(env, napi_set_named_property(env, exports, "MyObject", ctor)); | | 36 | + return exports; | | 37 | +} |
Collapse file Copy file name to clipboard +8Lines changed: 8 additions & 0 deletions | Original file line number | Diff line number | Diff line change |
|---|
| | 1 | +{ | | 2 | + 'targets': [ | | 3 | + { | | 4 | + 'target_name': 'binding', | | 5 | + 'sources': [ 'binding.c' ] | | 6 | + } | | 7 | + ] | | 8 | +} |
Collapse file Copy file name to clipboard +14Lines changed: 14 additions & 0 deletions | Original file line number | Diff line number | Diff line change |
|---|
| | 1 | +'use strict'; | | 2 | +// Flags: --expose-gc | | 3 | + | | 4 | +process.env.NODE_TEST_KNOWN_GLOBALS = 0; | | 5 | + | | 6 | +const common = require('../../common'); | | 7 | +const binding = require(`./build/${common.buildType}/binding`); | | 8 | + | | 9 | +global.it = new binding.MyObject(); | | 10 | + | | 11 | +global.cleanup = () => { | | 12 | + delete global.it; | | 13 | + global.gc(); | | 14 | +}; |
|