src: schedule unref immediate for uv_close in destructors by apapirovski · Pull Request #18307 · nodejs/node · GitHub

Calling uv_close directly in destructors seems to be problematic as it can bring a loop back alive after EmitBeforeExit. The problem is that this only happens sometimes as it depends on GC timing, making the behaviour of these two classes unpredictable.

This PR should fix that behaviour and also (I think?) fix the flaky sequential/test-async-wrap-getasyncid.js test.

Fixes: #18190

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

src

CI:
https://ci.nodejs.org/job/node-test-pull-request/12677/
https://ci.nodejs.org/job/node-test-pull-request/12678/
https://ci.nodejs.org/job/node-test-pull-request/12681/

Stress test CI:
https://ci.nodejs.org/job/node-stress-single-test/1747/
https://ci.nodejs.org/job/node-stress-single-test/1748/

So far no failures of the test in question... let's hope that remains the case.

CI & stress test are both clean.

Can we also make this deterministic by adding explicit gc to the test?

That feels less complex and I'd like to be careful about delaying resource cleanup in an unrefed resource; I think this would e.g. lead to a memory leak in embedders that just wait for the loop to finish?

@addaleax Sorry, missed your comment. The problem is that these uv_close callbacks might or might not run in their code. The fact that they're triggered by GC means that they might get triggered after the loop has already shutdown which means they won't execute. It seems tricky either way.

(For example, we also don't call uv_close on a bunch of internal handles like the ones for Immediates because by the time we know to do that it's already too late.)

To be clear, I agree this is far from ideal but I also don't like papering over the issue by calling gc explicitly in the test. I'll think on this a bit longer.

@apapirovski An embedder would run the loop until its empty if it wants proper cleanup … which is not really exposed by Node right now, that’s true.

I know on some level this doesn’t “count”, but it’s what my Workers implementation (in Ayo) does, and I’d like to PR that some time in the next 1 or 2 months or so; for that I would basically have to re-do this PR with that approach. (Which is fine! I can’t expect you to do work just because my PR might need it. 😄)

@addaleax Just so I'm not misunderstanding, wouldn't the cleanup code run in that case since the unrefed Immediate would run? (Just asking so I have full clarity on this, not trying to poke holes or something. I might be misunderstanding what we're discussing.)

But either way, I'll keep thinking on this. I wasn't super happy with this solution but I'm also not super happy with the current approach.

@apapirovski I guess you are right… my line of thinking was that there could be a situation where only the unrefed immediate was would be active, so the loop would stop. But if we’re spinning it anyway afterwards until everything is closed that doesn’t really matter…

Maybe the real question here is what the API contract for beforeExit is? I.e., whether the thing that resurrects the loop always needs to be visible to userland?

LGTM if you want to land this

I think I’d still prefer the other approach, but I’ll keep thinking about it as well

Maybe the real question here is what the API contract for beforeExit is? I.e., whether the thing that resurrects the loop always needs to be visible to userland?

I don't think it needs to be strictly "visible" but we should minimize arbitrary awakening via stuff like GC (since it's so heavily tied to timing and can also differ from platform to platform). Even just for the sake of avoiding false positives or negatives in our own tests. (For example, something like this can lead to a test involving an unintentionally unref'd handle succeeding because the GC keeps the loop alive for one more run.)

Anyway, I'm marking as blocked for now. I appreciate the approval but I do think this needs to be thought through a bit more on my part.

Now that I look at it... calling Stop() here is probably a minor (if harmless) bug as it in turn calls MakeWeak() when the destructor is almost certainly invoked from a weak callback.

You didn't introduce it so it's fine to leave it be for now but I figured I'd point it out.

I'm curious - was this caused by the uv_run changes in 9e08695?

@Fishrock123 It's likely but we would've had other issues due to the implementation before. That change was before my time but on the whole it seems positive after reviewing it in detail.

Ultimately, we just need to be more mindful of GC-triggered operations. But still not sure of a good solution. 🤔

Ultimately, we just need to be more mindful of GC-triggered operations. But still not sure of a good solution. 🤔

By the way, I think for some things we could also use a GCEpilogueCallback instead of an event-loop-based deferral … for cleanup from destructors that might be okay?

I think we’d even be allowed to call into JS in such a callback, but I would really prefer not to; having async_hooks callbacks fire at what is essentially a completely arbitrary point in your program doesn’t seem like a good idea…

What shall we do here to progress further?

Ping @apapirovski

@BridgeAR Your guess is as good as mine. I don't love this solution. The issue is an extreme edge case and I haven't seen it on our CI lately. I would prefer to keep it open, if nothing else then as a reminder to myself to continue investigating.

Investigating further would be great :)

I am going to close this for now. I guess it would be best to open an issue if a reminder is still necessary.

@apapirovski please feel free to reopen in case you want to work on it again.

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>

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>

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>

apapirovski added 2 commits January 22, 2018 21:17

cares_wrap: use unref immediate to uv_close in destructor

c939d4e

fs: use unref immediate to uv_close in destructor

9b5e671

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

apapirovski requested a review from bnoordhuis January 23, 2018 02:26

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 Jan 23, 2018

jasnell requested a review from addaleax January 23, 2018 19:43

fixup: add nullptr check

fae83ae

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

addaleax approved these changes Feb 1, 2018

View reviewed changes

apapirovski added the blocked PRs that are blocked by other issues or PRs. label Feb 1, 2018

bnoordhuis approved these changes Feb 5, 2018

View reviewed changes

apapirovski mentioned this pull request Feb 12, 2018

test: refactor test-async-wrap-getasyncid #18727

Closed

2 tasks

BridgeAR closed this May 15, 2018

This was referenced Jun 1, 2018

Investigate flaky sequential/test-async-wrap-getasyncid #18190

Closed

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

Closed

		@@ -74,7 +69,12 @@ StatWatcher::StatWatcher(Environment* env, Local<Object> wrap)

		StatWatcher::~StatWatcher() {
		Stop();

src: schedule unref immediate for uv_close in destructors by apapirovski · Pull Request #18307 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: schedule unref immediate for uv_close in destructors - #18307

src: schedule unref immediate for uv_close in destructors#18307apapirovski wants to merge 3 commits intonodejs:masternodejs/node:masterfrom apapirovski:fix-cares-wrap-wake-up-loopapapirovski/node:fix-cares-wrap-wake-up-loopCopy head branch name to clipboard

Conversation

apapirovski commented Jan 23, 2018

Checklist

Affected core subsystem(s)

Uh oh!

apapirovski commented Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apapirovski commented Jan 23, 2018

Uh oh!

addaleax commented Jan 28, 2018

Uh oh!

apapirovski commented Feb 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Feb 1, 2018

Uh oh!

apapirovski commented Feb 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Feb 1, 2018

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

apapirovski commented Feb 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis Feb 5, 2018

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Feb 10, 2018

Uh oh!

apapirovski commented Feb 12, 2018

Uh oh!

addaleax commented Feb 12, 2018

Uh oh!

BridgeAR commented Mar 2, 2018

Uh oh!

BridgeAR commented Apr 10, 2018

Uh oh!

apapirovski commented Apr 11, 2018

Uh oh!

BridgeAR commented Apr 22, 2018

Uh oh!

BridgeAR commented May 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

src: schedule unref immediate for uv_close in destructors#18307
apapirovski wants to merge 3 commits into
nodejs:masterfrom
apapirovski:fix-cares-wrap-wake-up-loop

apapirovski commented Jan 23, 2018 •
edited
Loading

apapirovski commented Feb 1, 2018 •
edited
Loading

apapirovski commented Feb 1, 2018 •
edited
Loading

apapirovski commented Feb 1, 2018 •
edited
Loading