src: manage handles more explicitly in cares_wrap, node_stat_watcher #21093

apapirovski · 2018-06-02T09:21:52Z

When garbage collecting ChannelWrap & StatWatcher directly call uv_close which can wake up the event loop when it's already going for a shutdown. Instead be more explicit about managing these handles throughout the lifetime of these two classes.

Refs: #18307
Fixes: #18190

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

apapirovski · 2018-06-02T09:22:26Z

CI: https://ci.nodejs.org/job/node-test-pull-request/15226/

apapirovski · 2018-06-02T09:23:29Z

FWIW this has been enabled by the addition of RunCleanup from @addaleax.

apapirovski · 2018-06-06T18:06:58Z

ping @addaleax — would appreciate a quick look, to make sure nothing too crazy is happening here. Thanks!

addaleax · 2018-06-06T18:11:12Z

src/cares_wrap.cc

+  env()->SetUnrefImmediate([](Environment* env, void* data) {
+    env->CloseHandle(reinterpret_cast<uv_timer_t*>(data),
+                     [](uv_timer_t* handle) { delete handle; });
+  }, timer_handle_);


So… what happens if we’re about to exit at this point? Do we just not close the handle? That creates a memory leak, i.e. I think we’d want to also close the handle from the ChannelWrap destructor.

Correct me if I'm wrong but if it's too late to run the loop then we won't get to close it properly anyway? This is inside the destructor already... we're kind of at the mercy of the garbage collector timing.

It’s not really too late to run the loop though, right? We’re just not allowing callbacks into JS anymore when we’re cleaning up, which makes the cleanup invisible from the JS PoV (and that is ultimately the goal here, right?)

Like, if I remember correctly this kind of situation is the reason that we’re running CleanupHandles() at the end of the RunCleanup() loop in env.cc

Well, what we trying to avoid is emitting beforeExit, then having the GC run and resurrecting the loop again. As far as I can tell, SetUnrefImmediate works here except if the GC ran after the check stage during the uv_run inside the cleanup phase. I suppose we could check for can_call_into_js and either use Immediate or not depending on its state.

(This is all needed because we don't properly manage the lifecycle of either of these... which is unfortunate.)

Inside the cleanup phase we don’t really care about GC anymore I think, we’re destroying all C++ objects anyway?

I suppose we could check for can_call_into_js and either use Immediate or not depending on its state.

What happens if we schedule it via SetUnrefImmediate() but never run that immediate because we don’t have anything else to clean up, so nothing refs the event loop?

Maybe another question: Do we ever want CloseHandle() to resurrect the loop’s “liveness”?

^ Yes, that's what I'm looking at now. Let me update this with something slightly different and see what you think :)

addaleax · 2018-06-06T18:11:21Z

src/node_stat_watcher.cc

+  env()->SetUnrefImmediate([](Environment* env, void* data) {
+    env->CloseHandle(reinterpret_cast<uv_fs_poll_t*>(data),
+                     [](uv_fs_poll_t* handle) { delete handle; });
+  }, watcher_);


Same situation here

addaleax · 2018-06-06T18:15:40Z

Btw, now that the workers PR has landed it’s a lot easier to confirm that leak: python tools/test.py --worker test/sequential/test-async-wrap-getasyncid.js does the trick :)

The crash message doesn’t tell you which handles are still open when exiting yet, but I’m working on that.

apapirovski · 2018-06-06T20:46:18Z

CI: https://ci.nodejs.org/job/node-test-pull-request/15296/

apapirovski · 2018-06-06T20:59:36Z

@addaleax I updated this to more explicitly manage the handles inside those two classes. IMO that's going to be cleaner than what I had going on before.

addaleax · 2018-06-06T21:02:45Z

src/node_stat_watcher.cc

-    Stop();
-  }
-  env()->CloseHandle(watcher_, [](uv_fs_poll_t* handle) { delete handle; });
+  CHECK_EQ(watcher_, nullptr);


Can you explain how we can be sure of this? Is this called somewhere besides from JS that I don’t see?

Also, as a side note: I think StatWatcher should just be a HandleWrap, if I’m not missing anything. That would solve this issue automatically.

It has to have Stop called on it to reach the destructor in the first place, as far as I can tell. Is there an edge case I'm missing?

Even without unref, yes, for Workers that’s the case:

'use strict'; const { Worker, isMainThread, parentPort } = require('worker_threads'); const fs = require('fs'); if (isMainThread) { const worker = new Worker(__filename); worker.once('message', () => worker.terminate()); } else { const watcher = fs.watchFile(__filename, () => {}); parentPort.postMessage('running'); }

I’ll PR that as a test file if that’s alright with you.

Ah, that makes sense. Ok, let me update this to account for that. That said, it should still solve the GC issue.

addaleax · 2018-06-06T21:03:39Z

src/cares_wrap.cc

+  if (timer_handle_ == nullptr)
+    return;
+
+  env()->CloseHandle(timer_handle_, [](uv_timer_t* handle) { delete handle; });


tbh, I don’t quite understand why this side-steps the beforeExit problematic? This can still be called from GC, right?

In like 99.99% of cases, no. This is for the absolute edge cases where there's an error and we live through it. In most cases it'll get cleared via ares_sockstate_cb.

apapirovski · 2018-06-06T21:33:24Z

Ok, I think this should be correct now @addaleax?

addaleax

Thanks!

bnoordhuis · 2018-06-07T09:39:02Z

src/cares_wrap.cc

-  timer_handle_->data = static_cast<void*>(this);
-  uv_timer_init(env()->event_loop(), timer_handle_);
+void ChannelWrap::StartTimer() {
+  if (timer_handle_ == nullptr) {


if (timer_handle_ != nullptr) return; - saves a level of indent.

It's a bit more complex if you look below. We also check if the handle is active in an else if. (We need to be able to call uv_timer_start to restart the timer if it expired but the handle still exists.)

bnoordhuis · 2018-06-07T09:40:29Z

src/node_stat_watcher.cc

@@ -154,6 +150,9 @@ void StatWatcher::Start(const FunctionCallbackInfo<Value>& args) {
  const uint32_t interval = args[2].As<Uint32>()->Value();

  // Safe, uv_ref/uv_unref are idempotent.


Can you move the comment down?

bnoordhuis · 2018-06-07T09:41:58Z

src/node_stat_watcher.cc

@@ -154,6 +150,9 @@ void StatWatcher::Start(const FunctionCallbackInfo<Value>& args) {
  const uint32_t interval = args[2].As<Uint32>()->Value();

  // Safe, uv_ref/uv_unref are idempotent.
+  wrap->watcher_ = new uv_fs_poll_t();
+  uv_fs_poll_init(wrap->env()->event_loop(), wrap->watcher_);


Since you're here, maybe CHECK_EQ(0, uv_fs_poll_init(...));

joyeecheung

the stats watcher part LGTM

joyeecheung · 2018-06-07T10:13:48Z

src/node_stat_watcher.cc

@@ -184,7 +183,8 @@ void StatWatcher::Stop(const FunctionCallbackInfo<Value>& args) {


 void StatWatcher::Stop() {
-  uv_fs_poll_stop(watcher_);
+  env()->CloseHandle(watcher_, [](uv_fs_poll_t* handle) { delete handle; });


um, wait, is it enough to replace uv_fs_poll_stop with uv_close?

apapirovski · 2018-06-07T12:18:34Z

CI: https://ci.nodejs.org/job/node-test-pull-request/15312/

Instead of relying on garbage collection to close the timer handle, manage its state more explicitly.

Instead of relying on garbage collection to close the timer handle, manage its state more explicitly. PR-URL: #21093 Fixes: #18190 Refs: #18307 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Instead of relying on garbage collection to close the handle, manage its state more explicitly. PR-URL: #21093 Fixes: #18190 Refs: #18307 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

PR-URL: #21093 Fixes: #18190 Refs: #18307 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Wrapping libuv handles is what `HandleWrap` is there for. This allows a decent reduction of state tracking machinery by moving active-ness tracking to JS, and removing all interaction with garbage collection. Refs: #21093 PR-URL: #21244 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

This Pull Request updates dependency [node](https://github.com/nodejs/node) from `v10.4.1` to `v10.5.0` <details> <summary>Release Notes</summary> ### [`v10.5.0`](https://github.com/nodejs/node/releases/v10.5.0) [Compare Source](nodejs/node@v10.4.1...v10.5.0) ##### Notable Changes * **crypto**: * Support for `crypto.scrypt()` has been added. [#20816](`nodejs/node#20816) * **fs**: * BigInt support has been added to `fs.stat` and `fs.watchFile`. [#20220](`nodejs/node#20220) * APIs that take `mode` as arguments no longer throw on values larger than `0o777`. [#20636](`nodejs/node#20636) [#20975](`nodejs/node#20975) (Fixes: [#20498](`nodejs/node#20498)) * Fix crashes in closed event watchers. [#20985](`nodejs/node#20985) (Fixes: [#20297](`nodejs/node#20297)) * **Worker Threads**: * Support for multi-threading has been added behind the `--experimental-worker` flag in the `worker_threads` module. This feature is *experimental* and may receive breaking changes at any time. [#20876](`nodejs/node#20876) ##### Commits * [[`a6986fe8b6`](nodejs/node@a6986fe8b6)] - **async_hooks**: remove deprecated example (Mathias Buus) [#20998](`nodejs/node#20998) * [[`4b9817bf1e`](nodejs/node@4b9817bf1e)] - **benchmark**: disable only the ESLint rule needing it (Rich Trott) [#21133](`nodejs/node#21133) * [[`ecba1c57b1`](nodejs/node@ecba1c57b1)] - **(SEMVER-MINOR)** **benchmark**: port cluster/echo to worker (Timothy Gu) [#20876](`nodejs/node#20876) * [[`02adb2d62c`](nodejs/node@02adb2d62c)] - **(SEMVER-MINOR)** **build**: expose openssl scrypt functions to addons (Ben Noordhuis) [#20816](`nodejs/node#20816) * [[`c3fbac432f`](nodejs/node@c3fbac432f)] - **build**: install markdown linter for travis (Richard Lau) [#21215](`nodejs/node#21215) * [[`896017b134`](nodejs/node@896017b134)] - **build**: build addon tests in parallel (Anna Henningsen) [#21155](`nodejs/node#21155) * [[`76927fc734`](nodejs/node@76927fc734)] - **build**: stop distclean from deleting v8 files (Ujjwal Sharma) [#21164](`nodejs/node#21164) * [[`b044256f2a`](nodejs/node@b044256f2a)] - **build**: use LC\_ALL of C for maximum compatibility (Rich Trott) [#21222](`nodejs/node#21222) * [[`78c7d666fb`](nodejs/node@78c7d666fb)] - **build**: don't change locale on smartos (Refael Ackermann) [#21220](`nodejs/node#21220) * [[`c688a00a6d`](nodejs/node@c688a00a6d)] - **build**: fix 'gas\_version' check on localized environments (Evandro Oliveira) [#20394](`nodejs/node#20394) * [[`79b3423fb5`](nodejs/node@79b3423fb5)] - **build**: initial .travis.yml implementation (Anna Henningsen) [#21059](`nodejs/node#21059) * [[`ea4be72f22`](nodejs/node@ea4be72f22)] - **child_process**: swallow errors in internal communication (Anatoli Papirovski) [#21108](`nodejs/node#21108) * [[`9981220e2a`](nodejs/node@9981220e2a)] - **crypto**: fix behavior of createCipher in wrap mode (Tobias Nießen) [#21287](`nodejs/node#21287) * [[`d0cb9cbb35`](nodejs/node@d0cb9cbb35)] - **(SEMVER-MINOR)** **crypto**: drop Math.pow(), use static exponentation (Ben Noordhuis) [#20816](`nodejs/node#20816) * [[`2d9c3cc89d`](nodejs/node@2d9c3cc89d)] - **(SEMVER-MINOR)** **crypto**: refactor randomBytes() (Ben Noordhuis) [#20816](`nodejs/node#20816) * [[`6262fa44d6`](nodejs/node@6262fa44d6)] - **(SEMVER-MINOR)** **crypto**: refactor pbkdf2() and pbkdf2Sync() methods (Ben Noordhuis) [#20816](`nodejs/node#20816) * [[`c9b4592dbf`](nodejs/node@c9b4592dbf)] - **(SEMVER-MINOR)** **crypto**: add scrypt() and scryptSync() methods (Ben Noordhuis) [#20816](`nodejs/node#20816) * [[`495756264a`](nodejs/node@495756264a)] - **(SEMVER-MINOR)** **crypto**: DRY type checking (Ben Noordhuis) [#20816](`nodejs/node#20816) * [[`e4a7e0d28b`](nodejs/node@e4a7e0d28b)] - **deps**: float ea7abee from openssl / CVE-2018-0732 (Rod Vagg) [#21282](`nodejs/node#21282) * [[`0b90b071c4`](nodejs/node@0b90b071c4)] - **deps**: Upgrade node-inspect to 1.11.5 (Jan Krems) [#21055](`nodejs/node#21055) * [[`ffc29c12da`](nodejs/node@ffc29c12da)] - **deps**: patch V8 to 6.7.288.46 (Myles Borins) [#21260](`nodejs/node#21260) * [[`14bb905d18`](nodejs/node@14bb905d18)] - **deps**: V8: cherry-pick a440efb27f from upstream (Yang Guo) [#21022](`nodejs/node#21022) * [[`65b9c427ac`](nodejs/node@65b9c427ac)] - **dns**: improve setServers() errors and performance (Jamie Davis) [#20445](`nodejs/node#20445) * [[`bc20ec0c0f`](nodejs/node@bc20ec0c0f)] - **doc**: eliminate \_you\_ from N-API doc (Rich Trott) [#21382](`nodejs/node#21382) * [[`318d6831bf`](nodejs/node@318d6831bf)] - **doc**: use imperative in COLLABORATOR\_GUIDE (Rich Trott) [#21340](`nodejs/node#21340) * [[`177a7c06a8`](nodejs/node@177a7c06a8)] - **doc**: remove obsolete wiki references from BUILDING (Rich Trott) [#21369](`nodejs/node#21369) * [[`15023df050`](nodejs/node@15023df050)] - **doc**: add davisjam to collaborators (Jamie Davis) [#21273](`nodejs/node#21273) * [[`17c21b67ac`](nodejs/node@17c21b67ac)] - **doc**: fix indentation in console.md (Vse Mozhet Byt) [#21367](`nodejs/node#21367) * [[`ef74368416`](nodejs/node@ef74368416)] - **doc**: fix heading of optional console method args (Michaël Zasso) [#21311](`nodejs/node#21311) * [[`4f17841c20`](nodejs/node@4f17841c20)] - **doc**: use Class Method label consistently (Rich Trott) [#21357](`nodejs/node#21357) * [[`4566ebacf4`](nodejs/node@4566ebacf4)] - **doc**: wrap style guide at 80 characters (Rich Trott) [#21361](`nodejs/node#21361) * [[`6c41f33571`](nodejs/node@6c41f33571)] - **doc**: wrap pull-requests.md at 80 characters (Rich Trott) [#21361](`nodejs/node#21361) * [[`b8213f17cc`](nodejs/node@b8213f17cc)] - **doc**: remove linking of url text to url (Rich Trott) [#21361](`nodejs/node#21361) * [[`3f78220c2b`](nodejs/node@3f78220c2b)] - **doc**: correct styling of \_GitHub\_ in onboarding doc (Rich Trott) [#21361](`nodejs/node#21361) * [[`9e994cb119`](nodejs/node@9e994cb119)] - **doc**: wrap releases.md at 80 chars (Rich Trott) [#21361](`nodejs/node#21361) * [[`e00e5e6d5d`](nodejs/node@e00e5e6d5d)] - **doc**: switch the order of Writable and Readable (Joseph Gordon) [#21333](`nodejs/node#21333) * [[`e1b571d6b7`](nodejs/node@e1b571d6b7)] - **doc**: make Deprecation cycle explanation more brief (Rich Trott) [#21303](`nodejs/node#21303) * [[`df0f7a3b4d`](nodejs/node@df0f7a3b4d)] - **doc**: clarify async execute callback usage (Michael Dawson) [#21217](`nodejs/node#21217) * [[`c5a65594ef`](nodejs/node@c5a65594ef)] - **doc**: move 5 collaborators to emeritus status (Rich Trott) [#21272](`nodejs/node#21272) * [[`c1d53f86f8`](nodejs/node@c1d53f86f8)] - **doc**: update NODE\_OPTIONS section in cli.md (Vse Mozhet Byt) [#21229](`nodejs/node#21229) * [[`13fd09bfa7`](nodejs/node@13fd09bfa7)] - **doc**: add build wg info to releases.md (Jon Moss) [#21275](`nodejs/node#21275) * [[`0da910f9a5`](nodejs/node@0da910f9a5)] - **doc**: move Italo A. Casas to Release Emeritus (Myles Borins) [#21315](`nodejs/node#21315) * [[`6f7de0b8d9`](nodejs/node@6f7de0b8d9)] - **doc**: trim deprecation level definition text (Rich Trott) [#21241](`nodejs/node#21241) * [[`dd2fc90dcf`](nodejs/node@dd2fc90dcf)] - **doc**: fix reference to workerData in worker\_threads (Jeremiah Senkpiel) [#21180](`nodejs/node#21180) * [[`5e46c16371`](nodejs/node@5e46c16371)] - **doc**: fix type in stream doc (Aliaksei Tuzik) [#21178](`nodejs/node#21178) * [[`85dc9ac418`](nodejs/node@85dc9ac418)] - **doc**: add Michaël Zasso to Release team (Michaël Zasso) [#21114](`nodejs/node#21114) * [[`5fa5ab6c48`](nodejs/node@5fa5ab6c48)] - **doc**: naming function as suggested in addon docs (Tommaso Allevi) [#21067](`nodejs/node#21067) * [[`fe5d35123b`](nodejs/node@fe5d35123b)] - **(SEMVER-MINOR)** **doc**: document BigInt support in fs.Stats (Joyee Cheung) [#20220](`nodejs/node#20220) * [[`2c4f80ffba`](nodejs/node@2c4f80ffba)] - **doc**: remove spaces around slashes (Rich Trott) [#21140](`nodejs/node#21140) * [[`72e7e1da2d`](nodejs/node@72e7e1da2d)] - **doc**: alphabetize tls options (Rich Trott) [#21139](`nodejs/node#21139) * [[`06ac81e786`](nodejs/node@06ac81e786)] - **doc**: streamline errors.md introductory material (Rich Trott) [#21138](`nodejs/node#21138) * [[`73b8975b41`](nodejs/node@73b8975b41)] - **doc**: simplify deprecation language (Rich Trott) [#21136](`nodejs/node#21136) * [[`6caa354377`](nodejs/node@6caa354377)] - **(SEMVER-MINOR)** **doc**: explain Worker semantics in async\_hooks.md (Anna Henningsen) [#20876](`nodejs/node#20876) * [[`9f9355d6d2`](nodejs/node@9f9355d6d2)] - **doc**: fix inconsistent documentation (host vs hostname) (Davis Okoth) [#20933](`nodejs/node#20933) * [[`a5c571424a`](nodejs/node@a5c571424a)] - **doc**: document file mode caveats on Windows (Joyee Cheung) [#20636](`nodejs/node#20636) * [[`a75e44d135`](nodejs/node@a75e44d135)] - **esm**: ensure require.main for CJS top-level loads (Guy Bedford) [#21150](`nodejs/node#21150) * [[`04e8f0749e`](nodejs/node@04e8f0749e)] - **(SEMVER-MINOR)** **fs**: support BigInt in fs.\*stat and fs.watchFile (Joyee Cheung) [#20220](`nodejs/node#20220) * [[`c09bfd81b7`](nodejs/node@c09bfd81b7)] - **fs**: do not crash when using a closed fs event watcher (Joyee Cheung) [#20985](`nodejs/node#20985) * [[`bacb2cb550`](nodejs/node@bacb2cb550)] - **fs**: refactor fs module (James M Snell) [#20764](`nodejs/node#20764) * [[`db0bb5214a`](nodejs/node@db0bb5214a)] - **fs**: improve fchmod{Sync} validation (cjihrig) [#20588](`nodejs/node#20588) * [[`2ffb9d6b5c`](nodejs/node@2ffb9d6b5c)] - **fs**: drop duplicate API in promises mode (Сковорода Никита Андреевич) [#20559](`nodejs/node#20559) * [[`fc0b3610e2`](nodejs/node@fc0b3610e2)] - **fs**: don't limit ftruncate() length to 32 bits (cjihrig) [#20851](`nodejs/node#20851) * [[`469baa062e`](nodejs/node@469baa062e)] - **fs**: add length validation to fs.truncate() (cjihrig) [#20851](`nodejs/node#20851) * [[`6aade4a765`](nodejs/node@6aade4a765)] - **http**: remove a pair of outdated comments (Mark S. Everitt) [#21214](`nodejs/node#21214) * [[`bcaf59c739`](nodejs/node@bcaf59c739)] - **http2**: fix memory leak for uncommon headers (Anna Henningsen) [#21336](`nodejs/node#21336) * [[`dee250fd77`](nodejs/node@dee250fd77)] - **http2**: safer Http2Session destructor (Anatoli Papirovski) [#21194](`nodejs/node#21194) * [[`296fd57324`](nodejs/node@296fd57324)] - **inspector**: stop dragging platform pointer (Eugene Ostroukhov) * [[`fb71337bdf`](nodejs/node@fb71337bdf)] - **(SEMVER-MINOR)** **lib**: rename checkIsArrayBufferView() (Ben Noordhuis) [#20816](`nodejs/node#20816) * [[`f3570f201b`](nodejs/node@f3570f201b)] - **(SEMVER-MINOR)** **lib**: replace checkUint() with validateInt32() (Ben Noordhuis) [#20816](`nodejs/node#20816) * [[`b4b7d368be`](nodejs/node@b4b7d368be)] - **lib**: unmask mode\_t values with 0o777 (Joyee Cheung) [#20975](`nodejs/node#20975) * [[`36e5100a39`](nodejs/node@36e5100a39)] - **lib**: support ranges in validateInt32() (cjihrig) [#20588](`nodejs/node#20588) * [[`2fe88d2218`](nodejs/node@2fe88d2218)] - **lib**: mask mode\_t type of arguments with 0o777 (Joyee Cheung) [#20636](`nodejs/node#20636) * [[`a0cfb0c9d4`](nodejs/node@a0cfb0c9d4)] - **lib**: add validateInteger() validator (cjihrig) [#20851](`nodejs/node#20851) * [[`740d9f1a0e`](nodejs/node@740d9f1a0e)] - **lib,src**: make `StatWatcher` a `HandleWrap` (Anna Henningsen) [#21244](`nodejs/node#21244) * [[`a657984109`](nodejs/node@a657984109)] - **lib,src**: remove openssl feature conditionals (Ben Noordhuis) [#21094](`nodejs/node#21094) * [[`653b20b26d`](nodejs/node@653b20b26d)] - **loader**: remove unused error code in module\_job (Gus Caplan) [#21354](`nodejs/node#21354) * [[`5d3dfedca2`](nodejs/node@5d3dfedca2)] - **meta**: remove CODEOWNERS (Rich Trott) [#21161](`nodejs/node#21161) * [[`169bff3e9e`](nodejs/node@169bff3e9e)] - **n-api**: name CallbackBundle function fields (Anna Henningsen) [#21240](`nodejs/node#21240) * [[`1dc9330b3a`](nodejs/node@1dc9330b3a)] - **n-api**: improve runtime perf of n-api func call (Kenny Yuan) [#21072](`nodejs/node#21072) * [[`9047c8182c`](nodejs/node@9047c8182c)] - **n-api**: remove unused napi\_env member (Gabriel Schulhof) [#21127](`nodejs/node#21127) * [[`18c057ab26`](nodejs/node@18c057ab26)] - **net**: emit 'close' when socket ends before connect (Brett Kiefer) [#21290](`nodejs/node#21290) * [[`a3fd1cd8ea`](nodejs/node@a3fd1cd8ea)] - **perf_hooks**: remove less useful bootstrap marks (James M Snell) [#21247](`nodejs/node#21247) * [[`8fddf591c5`](nodejs/node@8fddf591c5)] - **perf_hooks**: set bootstrap complete in only one place (James M Snell) [#21247](`nodejs/node#21247) * [[`fc2956d37a`](nodejs/node@fc2956d37a)] - **process**: backport process/methods file (Michaël Zasso) [#21172](`nodejs/node#21172) * [[`78ad4e9dde`](nodejs/node@78ad4e9dde)] - **src**: remove unused argc var in node\_stat\_watcher (Daniel Bevenius) [#21337](`nodejs/node#21337) * [[`7fa1344143`](nodejs/node@7fa1344143)] - **src**: use `%zx` in printf for size\_t (Anna Henningsen) [#21323](`nodejs/node#21323) * [[`671346ee8f`](nodejs/node@671346ee8f)] - **src**: do proper error checking in `AsyncWrap::MakeCallback` (Anna Henningsen) [#21189](`nodejs/node#21189) * [[`aa468abc4c`](nodejs/node@aa468abc4c)] - **src**: unify native symbol inspection code (Anna Henningsen) [#21238](`nodejs/node#21238) * [[`e92b89a75d`](nodejs/node@e92b89a75d)] - **src**: fix http2 typos (Anatoli Papirovski) [#21194](`nodejs/node#21194) * [[`4f01168414`](nodejs/node@4f01168414)] - **src**: do not persist fs\_poll handle in stat\_watcher (Anatoli Papirovski) [#21093](`nodejs/node#21093) * [[`685b9b2a6a`](nodejs/node@685b9b2a6a)] - **src**: do not persist timer handle in cares\_wrap (Anatoli Papirovski) [#21093](`nodejs/node#21093) * [[`4757771db3`](nodejs/node@4757771db3)] - **src**: add consistency check to node\_platform.cc (Anna Henningsen) [#21156](`nodejs/node#21156) * [[`8e2e16721b`](nodejs/node@8e2e16721b)] - **src**: add node\_encoding.cc (James M Snell) [#21112](`nodejs/node#21112) * [[`39b38754eb`](nodejs/node@39b38754eb)] - **src**: cleanup beforeExit for consistency (James M Snell) [#21113](`nodejs/node#21113) * [[`314b47d1cf`](nodejs/node@314b47d1cf)] - **(SEMVER-MINOR)** **src**: add Env::profiler\_idle\_notifier\_started() (Timothy Gu) [#20876](`nodejs/node#20876) * [[`5209ff9562`](nodejs/node@5209ff9562)] - **(SEMVER-MINOR)** **src**: remove unused fields msg\_ and env\_ (Daniel Bevenius) [#20876](`nodejs/node#20876) * [[`9a734132f9`](nodejs/node@9a734132f9)] - **(SEMVER-MINOR)** **src**: make handle onclose property a Symbol (Anna Henningsen) [#20876](`nodejs/node#20876) * [[`e6f06807b1`](nodejs/node@e6f06807b1)] - **(SEMVER-MINOR)** **src**: simplify handle closing (Anna Henningsen) [#20876](`nodejs/node#20876) * [[`65924c70e8`](nodejs/node@65924c70e8)] - **(SEMVER-MINOR)** **src**: remove unused fields isolate\_ (Daniel Bevenius) [#20876](`nodejs/node#20876) * [[`de7403f813`](nodejs/node@de7403f813)] - **(SEMVER-MINOR)** **src**: cleanup per-isolate state on platform on isolate unregister (Anna Henningsen) [#20876](`nodejs/node#20876) * [[`ba17c9e46b`](nodejs/node@ba17c9e46b)] - **src**: refactor bootstrap to use bootstrap object (James M Snell) [#20917](`nodejs/node#20917) * [[`cbdc1fdf44`](nodejs/node@cbdc1fdf44)] - **src, tools**: add check for left leaning pointers (Daniel Bevenius) [#21010](`nodejs/node#21010) * [[`935309325b`](nodejs/node@935309325b)] - **test**: fix deprecation warning due to util.print (Tobias Nießen) [#21265](`nodejs/node#21265) * [[`d7ba75f8aa`](nodejs/node@d7ba75f8aa)] - **test**: add test to check colorMode type of Console (Masashi Hirano) [#21248](`nodejs/node#21248) * [[`0b00172df8`](nodejs/node@0b00172df8)] - **test**: removing unnecessary parameter from assert call (djmgit) [#21307](`nodejs/node#21307) * [[`dea3ac7bff`](nodejs/node@dea3ac7bff)] - **test**: improve statwatcher async\_hooks test (Anna Henningsen) [#21244](`nodejs/node#21244) * [[`792335f712`](nodejs/node@792335f712)] - **test**: add workerdata-sharedarraybuffer test (Jeremiah Senkpiel) [#21180](`nodejs/node#21180) * [[`e8d15cb149`](nodejs/node@e8d15cb149)] - **test**: mark test-inspector-port-zero-cluster flaky (Rich Trott) [#21251](`nodejs/node#21251) * [[`688bdfef7f`](nodejs/node@688bdfef7f)] - **test**: add crypto check to test-http2-debug (Daniel Bevenius) [#21205](`nodejs/node#21205) * [[`2270ab2a12`](nodejs/node@2270ab2a12)] - **test**: remove string literals from assert.strictEqual() calls (James Kylstra) [#21211](`nodejs/node#21211) * [[`187951c0fc`](nodejs/node@187951c0fc)] - **test**: move inspector-stress-http to sequential (Rich Trott) [#21227](`nodejs/node#21227) * [[`bda34ea203`](nodejs/node@bda34ea203)] - **test**: check gc does not resurrect the loop (Anatoli Papirovski) [#21093](`nodejs/node#21093) * [[`4d782c4720`](nodejs/node@4d782c4720)] - **test**: improve assert error messages (Hristijan Gjorgjievski) [#21160](`nodejs/node#21160) * [[`2655c7b194`](nodejs/node@2655c7b194)] - **test**: mark fs-readfile-tostring-fail flaky for all (Rich Trott) [#21177](`nodejs/node#21177) * [[`17954c2b01`](nodejs/node@17954c2b01)] - **test**: improve internal/buffer.js test coverage (Masashi Hirano) [#21061](`nodejs/node#21061) * [[`2ff4704447`](nodejs/node@2ff4704447)] - **test**: move test-readuint to test-buffer-readuint (Michaël Zasso) [#21170](`nodejs/node#21170) * [[`9c3a7bf076`](nodejs/node@9c3a7bf076)] - **test**: make url-util-format engine agnostic (Rich Trott) [#21141](`nodejs/node#21141) * [[`3d8ec8f85c`](nodejs/node@3d8ec8f85c)] - **test**: make url-parse-invalid-input engine agnostic (Rich Trott) [#21132](`nodejs/node#21132) * [[`0b0370f884`](nodejs/node@0b0370f884)] - **test**: remove unref in http2 test (Anatoli Papirovski) [#21145](`nodejs/node#21145) * [[`14a017cf8d`](nodejs/node@14a017cf8d)] - **test**: apply promises API to fourth appendFile test (Rich Trott) [#21131](`nodejs/node#21131) * [[`aa9dbf666b`](nodejs/node@aa9dbf666b)] - **test**: apply promises API to fourth appendFile test (Rich Trott) [#21131](`nodejs/node#21131) * [[`185b9e45d3`](nodejs/node@185b9e45d3)] - **test**: apply promises API to third appendFile test (Rich Trott) [#21131](`nodejs/node#21131) * [[`c400448e85`](nodejs/node@c400448e85)] - **test**: improve debug output in trace-events test (Rich Trott) [#21120](`nodejs/node#21120) * [[`a4ad9891e3`](nodejs/node@a4ad9891e3)] - **test**: add test for Linux perf (Matheus Marchini) [#20783](`nodejs/node#20783) * [[`e16036c462`](nodejs/node@e16036c462)] - **test**: create new directory v8-updates (Matheus Marchini) [#20783](`nodejs/node#20783) * [[`93ce63c89f`](nodejs/node@93ce63c89f)] - **(SEMVER-MINOR)** **test**: add test against unsupported worker features (Timothy Gu) [#20876](`nodejs/node#20876) * [[`94dcdfb898`](nodejs/node@94dcdfb898)] - **test**: increase coverage for fs.promises.truncate (Masashi Hirano) [#20638](`nodejs/node#20638) * [[`c9cee63179`](nodejs/node@c9cee63179)] - **test,tools**: refactor custom ESLint for readability (Rich Trott) [#21134](`nodejs/node#21134) * [[`ed05d9a821`](nodejs/node@ed05d9a821)] - **(SEMVER-MINOR)** **test,tools**: enable running tests under workers (Anna Henningsen) [#20876](`nodejs/node#20876) * [[`6285fe94f6`](nodejs/node@6285fe94f6)] - **tools**: do not disable `quotes` rule in .eslintrc.js (Rich Trott) [#21338](`nodejs/node#21338) * [[`98346de08c`](nodejs/node@98346de08c)] - **tools**: lint doc/\*.md files (Rich Trott) [#21361](`nodejs/node#21361) * [[`521f8f1d95`](nodejs/node@521f8f1d95)] - **tools**: add BigInt64Array and BigUint64Array to globals (Joyee Cheung) [#21255](`nodejs/node#21255) * [[`a5c386d1ba`](nodejs/node@a5c386d1ba)] - **tools**: add option to use custom template with js2c.py (Shelley Vohr) [#21187](`nodejs/node#21187) * [[`7f70fe83ef`](nodejs/node@7f70fe83ef)] - **tools**: add BigInt to globals (Nikolai Vavilov) [#21237](`nodejs/node#21237) * [[`4e742e379b`](nodejs/node@4e742e379b)] - **tools**: update tooling to work with new macOS CLI … (Rich Trott) [#21173](`nodejs/node#21173) * [[`ed2b57bcd5`](nodejs/node@ed2b57bcd5)] - **tools**: remove unused global types from type-parser (Rich Trott) [#21135](`nodejs/node#21135) * [[`d46446afc5`](nodejs/node@d46446afc5)] - **v8**: replace Buffer with FastBuffer in deserialize (Ujjwal Sharma) [#21196](`nodejs/node#21196) * [[`917960e0a1`](nodejs/node@917960e0a1)] - **win, build**: add documentation support to vcbuild (Bartosz Sosnowski) [#19663](`nodejs/node#19663) * [[`03fbc9e749`](nodejs/node@03fbc9e749)] - **(SEMVER-MINOR)** **worker**: rename to worker\_threads (Anna Henningsen) [#20876](`nodejs/node#20876) * [[`9ad42b766e`](nodejs/node@9ad42b766e)] - **(SEMVER-MINOR)** **worker**: improve error (de)serialization (Anna Henningsen) [#20876](`nodejs/node#20876) * [[`6b1a887aa2`](nodejs/node@6b1a887aa2)] - **(SEMVER-MINOR)** **worker**: enable stdio (Anna Henningsen) [#20876](`nodejs/node#20876) * [[`c97fb91e55`](nodejs/node@c97fb91e55)] - **(SEMVER-MINOR)** **worker**: restrict supported extensions (Timothy Gu) [#20876](`nodejs/node#20876) * [[`109c92e8fa`](nodejs/node@109c92e8fa)] - **(SEMVER-MINOR)** **worker**: initial implementation (Anna Henningsen) [#20876](`nodejs/node#20876) * [[`d1f372f052`](nodejs/node@d1f372f052)] - **(SEMVER-MINOR)** **worker**: add `SharedArrayBuffer` sharing (Anna Henningsen) [#20876](`nodejs/node#20876) * [[`f447acd87b`](nodejs/node@f447acd87b)] - **(SEMVER-MINOR)** **worker**: support MessagePort passing in messages (Anna Henningsen) [#20876](`nodejs/node#20876) * [[`337be58ee6`](nodejs/node@337be58ee6)] - **(SEMVER-MINOR)** **worker**: implement `MessagePort` and `MessageChannel` (Anna Henningsen) [#20876](`nodejs/node#20876) * [[`4a54ebc3bd`](nodejs/node@4a54ebc3bd)] - **worker,src**: display remaining handles if `uv\_loop\_close` fails (Anna Henningsen) [#21238](`nodejs/node#21238) * [[`529d24e3e8`](nodejs/node@529d24e3e8)] - ***Revert*** "**workers,trace_events**: set thread name for workers" (James M Snell) [#21363](`nodejs/node#21363) * [[`dfb5cf6963`](nodejs/node@dfb5cf6963)] - **workers,trace_events**: set thread name for workers (James M Snell) [#21246](`nodejs/node#21246) --- </details> --- This PR has been generated by [Renovate Bot](https://renovatebot.com).

Refs: nodejs#21093 (comment)

Refs: #21093 (comment) PR-URL: #21179 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Refs: nodejs#21093 (comment) PR-URL: nodejs#21179 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Refs: #21093 (comment) PR-URL: #21179 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: James M Snell <jasnell@gmail.com>

apapirovski added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jun 2, 2018

apapirovski requested a review from addaleax June 2, 2018 09:21

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. fs Issues and PRs related to the fs subsystem / file system. labels Jun 2, 2018

apapirovski force-pushed the fix-destructor-revive branch from 80c17aa to 02b2663 Compare June 2, 2018 09:54

Fishrock123 approved these changes Jun 2, 2018

View reviewed changes

addaleax reviewed Jun 6, 2018

View reviewed changes

apapirovski changed the title ~~src: call uv_close in unrefed immediate~~ src: manage handles more explicitly in cares_wrap, node_stat_watcher Jun 6, 2018

apapirovski force-pushed the fix-destructor-revive branch from 02b2663 to 092d106 Compare June 6, 2018 20:39

apapirovski force-pushed the fix-destructor-revive branch 2 times, most recently from cc24297 to 6fd23be Compare June 6, 2018 20:53

addaleax reviewed Jun 6, 2018

View reviewed changes

addaleax mentioned this pull request Jun 6, 2018

test: add fs.watchFile() + worker.terminate() test #21179

Closed

3 tasks

addaleax approved these changes Jun 6, 2018

View reviewed changes

bnoordhuis approved these changes Jun 7, 2018

View reviewed changes

joyeecheung approved these changes Jun 7, 2018

View reviewed changes

joyeecheung reviewed Jun 7, 2018

View reviewed changes

cjihrig approved these changes Jun 7, 2018

View reviewed changes

apapirovski force-pushed the fix-destructor-revive branch from e059bf1 to 8906e29 Compare June 7, 2018 15:58

src: do not persist timer handle in cares_wrap

0bf2b28

Instead of relying on garbage collection to close the timer handle, manage its state more explicitly.

targos mentioned this pull request Jun 19, 2018

v10.5.0 proposal #21400

Merged

This was referenced Jun 20, 2018

2018-06-20 Version 10.5.0 (Current) @targos beevelop/docker-android-nodejs#216

Closed

2018-06-20 Version 10.5.0 (Current) @targos beevelop/docker-nodejs#201

Closed

addaleax added a commit to addaleax/node that referenced this pull request Mar 18, 2019

test: add fs.watchFile() + worker.terminate() test

3f00235

Refs: nodejs#21093 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: manage handles more explicitly in cares_wrap, node_stat_watcher #21093

src: manage handles more explicitly in cares_wrap, node_stat_watcher #21093

apapirovski commented Jun 2, 2018 •

edited

Loading

apapirovski commented Jun 2, 2018 •

edited

Loading

apapirovski commented Jun 2, 2018

apapirovski commented Jun 6, 2018

addaleax Jun 6, 2018

apapirovski Jun 6, 2018

addaleax Jun 6, 2018

apapirovski Jun 6, 2018 •

edited

Loading

addaleax Jun 6, 2018

addaleax Jun 6, 2018 •

edited

Loading

apapirovski Jun 6, 2018

addaleax Jun 6, 2018

addaleax commented Jun 6, 2018

apapirovski commented Jun 6, 2018 •

edited

Loading

apapirovski commented Jun 6, 2018

addaleax Jun 6, 2018

apapirovski Jun 6, 2018

addaleax Jun 6, 2018

apapirovski Jun 6, 2018

addaleax Jun 6, 2018

apapirovski Jun 6, 2018

apapirovski commented Jun 6, 2018

addaleax left a comment

bnoordhuis Jun 7, 2018

apapirovski Jun 7, 2018

bnoordhuis Jun 7, 2018

bnoordhuis Jun 7, 2018

joyeecheung left a comment

joyeecheung Jun 7, 2018

bnoordhuis Jun 7, 2018

apapirovski commented Jun 7, 2018

		@@ -154,6 +150,9 @@ void StatWatcher::Start(const FunctionCallbackInfo<Value>& args) {
		const uint32_t interval = args[2].As<Uint32>()->Value();

		// Safe, uv_ref/uv_unref are idempotent.

src: manage handles more explicitly in cares_wrap, node_stat_watcher #21093

src: manage handles more explicitly in cares_wrap, node_stat_watcher #21093

Conversation

apapirovski commented Jun 2, 2018 • edited Loading

Checklist

apapirovski commented Jun 2, 2018 • edited Loading

apapirovski commented Jun 2, 2018

apapirovski commented Jun 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Jun 6, 2018

apapirovski commented Jun 6, 2018 • edited Loading

apapirovski commented Jun 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski commented Jun 6, 2018

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski commented Jun 7, 2018

apapirovski commented Jun 2, 2018 •

edited

Loading

apapirovski commented Jun 2, 2018 •

edited

Loading

apapirovski Jun 6, 2018 •

edited

Loading

addaleax Jun 6, 2018 •

edited

Loading

apapirovski commented Jun 6, 2018 •

edited

Loading