src: schedule unref immediate for uv_close in destructors

[apapirovski]

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

[apapirovski]

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.

[apapirovski]

CI & stress test are both clean.

[addaleax]

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?

[apapirovski]

@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.

[addaleax]

@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. 😄)

[apapirovski]

@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.

[addaleax]

@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?

[addaleax]

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

[apapirovski]

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.

[bnoordhuis]

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.

[Fishrock123]

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

[apapirovski]

@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. 🤔

[addaleax]

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…

[BridgeAR]

What shall we do here to progress further?

[BridgeAR]

Ping @apapirovski

[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.

[BridgeAR]

Investigating further would be great :)

[BridgeAR]

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.