async_hooks: run destroy callbacks before normal exit #13286
Conversation
nodejs-github-bot
added
async_wrap
c++
addaleax
added
async_hooks
addaleax
force-pushed
the
fix-13262
branch
2 times, most recently
from
a86cbfd
to
2745e80
Compare
Run destroy callbacks before a normal application exit happens. Fixes: nodejs#13262
| @@ -164,8 +168,6 @@ static void DestroyIdsCb(uv_idle_t* handle) { | |||
| FatalException(env->isolate(), try_catch); | |||
| } | |||
| } | |||
|
|
|||
| env->destroy_ids_list()->clear(); | |||
There was a problem hiding this comment.
Removing this fixes the issue tested by parallel/test-async-hooks-close-during-destroy. If somebody feels like I should, I can split it into a second commit.
There was a problem hiding this comment.
This would be nice for the purpose of adding a good commit message descibing the issue.
There was a problem hiding this comment.
@addaleax If you don't clear the std::vector<double> then destroy ids list will continue to call the destroy hooks for all the previously entered asyncIds.
There was a problem hiding this comment.
@trevnorris Just before the loop above, you already swapped the list with an empty vector, which is the right way to solve that. Re-clearing the list may actually clear too much (you can try out the test file here yourself, if you like).
There was a problem hiding this comment.
Just before the loop above, you already swapped the list with an empty vector, which is the right way to solve that.
ah yup. you're right.
| @@ -4531,6 +4531,8 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, | |||
| } while (more == true); | |||
| } | |||
|
|
|||
| AsyncWrap::RunDestroyCbs(&env); | |||
There was a problem hiding this comment.
If destroy creates a new asynchronous resource (think logging) then that won't be executed. This is why I think we should treat the destroy queue exactly like the setImmediate queue, by using uv_check_start.
I also don't like to pollute the event loop code if it can be handled by libuv.
There was a problem hiding this comment.
Unfortunately that seems to change the timing of the destroy callback visibly and makes a couple of tests fail. I’ll try to get to it tomorrow.
There was a problem hiding this comment.
This is why I think we should treat the
destroyqueue exactly like thesetImmediatequeue, by usinguv_check_start.
You mean you'd prefer we use uv_check_t instead of a uv_idle_t as it's done in PushBackDestroyId() (src/async-wrap.cc)?
Honestly if you want the callbacks to run before the poll phase then it might be worth it to run it as a uv_timer_t with a timeout of 0. This will cause them to be run before any setImmediate()'s are called.
There was a problem hiding this comment.
@addaleax ah. think I see what's going on. will get another fix for this in a moment.
There was a problem hiding this comment.
@trevnorris Thanks – I’ll pick the env->destroy_ids_list()->clear(); changes above into another PR and let you take it from here. I’ll close this once your PR is up. 👍
|
|
||
| // Create a server to trigger the first `destroy` callback. | ||
| net.createServer().listen(0).close(); | ||
| srv2 = net.createServer().listen(0); |
There was a problem hiding this comment.
This abuses node internals. We should assume that bind() is always async, as if a hostname needed to be resolved. e.g. the following causes the server to never close:
net.createServer().listen({port: 0, host: 'localhost'}).close();Instead it needs to be written as such:
net.createServer().listen({port: 0, host: 'localhost'}, function() { this.close() });
This means you can't reliably call srv2.close() from the destroy callback the way is being done here.
There was a problem hiding this comment.
IOW, the those two lines should instead be written as such:
srv2 = net.createServer().listen(0, () => {
net.createServer().listen(0, function() { this.close() });
});There was a problem hiding this comment.
@trevnorris Is there anything in Node that always closes synchronously? Anyway, I’ve updated #13353 to just use AsyncResource instead, and I’d suggest we continue the review there.
There was a problem hiding this comment.
@addaleax everything libuv that "closes" always goes through uv_close() and so will be called async.
Remove a `.clear()` call on the list of destroy ids that may inadvertently swallow `destroy` events. The list is already cleared earlier in the `DestroyIdsCb` function, so usually this was a no-op; but when the garbage collection or its equivalent was active during a `destroy` hook itself, it was possible that `destroy` hooks were scheduled but cleared before the next event loop iteration in which they would have been emitted. Ref: nodejs#13286
|
#13369 replaces this. |
Run destroy callbacks before a normal application exit happens.
Fixes: #13262
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
async_hooks
/cc @AndreasMadsen