async_hooks: run destroy callbacks before normal exit #13286

addaleax · 2017-05-29T18:53:27Z

Run destroy callbacks before a normal application exit happens.

Fixes: #13262

Checklist

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

Affected core subsystem(s)

async_hooks

/cc @AndreasMadsen

Run destroy callbacks before a normal application exit happens. Fixes: nodejs#13262

addaleax · 2017-05-29T19:37:11Z

src/async-wrap.cc

@@ -164,8 +168,6 @@ static void DestroyIdsCb(uv_idle_t* handle) {
      FatalException(env->isolate(), try_catch);
    }
  }
-
-  env->destroy_ids_list()->clear();


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.

This would be nice for the purpose of adding a good commit message descibing the issue.

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

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

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.

AndreasMadsen · 2017-05-29T19:55:21Z

src/node.cc

@@ -4531,6 +4531,8 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data,
    } while (more == true);
  }

+  AsyncWrap::RunDestroyCbs(&env);


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.

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.

This is why I think we should treat the destroy queue exactly like the setImmediate queue, by using uv_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.

@addaleax ah. think I see what's going on. will get another fix for this in a moment.

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

trevnorris · 2017-05-31T23:08:54Z

test/parallel/test-async-hooks-close-during-destroy.js

+
+// Create a server to trigger the first `destroy` callback.
+net.createServer().listen(0).close();
+srv2 = net.createServer().listen(0);


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.

IOW, the those two lines should instead be written as such:

srv2 = net.createServer().listen(0, () => { net.createServer().listen(0, function() { this.close() }); });

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

@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

AndreasMadsen · 2017-06-02T09:45:45Z

#13369 replaces this.

nodejs-github-bot added async_wrap c++ Issues and PRs that require attention from people who are familiar with C++. labels May 29, 2017

addaleax force-pushed the fix-13262 branch from 5628e6a to 0a85e7c Compare May 29, 2017 18:53

addaleax added async_hooks Issues and PRs related to the async hooks subsystem. and removed async_wrap labels May 29, 2017

addaleax force-pushed the fix-13262 branch 2 times, most recently from a86cbfd to 2745e80 Compare May 29, 2017 19:20

async_hooks: run destroy cbs before normal exit

fb88c0d

Run destroy callbacks before a normal application exit happens. Fixes: nodejs#13262

addaleax force-pushed the fix-13262 branch from 2745e80 to fb88c0d Compare May 29, 2017 19:36

addaleax commented May 29, 2017

View reviewed changes

AndreasMadsen reviewed May 29, 2017

View reviewed changes

AndreasMadsen mentioned this pull request May 30, 2017

[async_hooks] tracking issue nodejs/diagnostics#29

Closed

27 tasks

addaleax mentioned this pull request May 31, 2017

async_wrap: remove erroneous destroy list clear() #13353

Closed

3 tasks

trevnorris reviewed May 31, 2017

View reviewed changes

trevnorris mentioned this pull request Jun 1, 2017

[async_wrap] call destroy hooks on uv_timer_t #13369

Merged

3 tasks

AndreasMadsen closed this Jun 2, 2017

AndreasMadsen added the dont-land-on-v6.x label Jul 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

async_hooks: run destroy callbacks before normal exit #13286

async_hooks: run destroy callbacks before normal exit #13286

addaleax commented May 29, 2017

addaleax May 29, 2017

AndreasMadsen May 29, 2017

trevnorris May 31, 2017

addaleax May 31, 2017

trevnorris Jun 1, 2017

AndreasMadsen May 29, 2017 •

edited

Loading

addaleax May 29, 2017

trevnorris May 31, 2017 •

edited

Loading

trevnorris May 31, 2017

addaleax May 31, 2017

trevnorris May 31, 2017

trevnorris May 31, 2017

addaleax May 31, 2017

trevnorris Jun 1, 2017

AndreasMadsen commented Jun 2, 2017

async_hooks: run destroy callbacks before normal exit #13286

async_hooks: run destroy callbacks before normal exit #13286

Conversation

addaleax commented May 29, 2017

Checklist

Affected core subsystem(s)

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

AndreasMadsen May 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevnorris May 31, 2017 • edited Loading

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

Choose a reason for hiding this comment

AndreasMadsen commented Jun 2, 2017

AndreasMadsen May 29, 2017 •

edited

Loading

trevnorris May 31, 2017 •

edited

Loading