[async_wrap] call destroy hooks on uv_timer_t #13369

trevnorris · 2017-06-01T12:28:08Z

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_wrap

Summary

Calling the destroy callbacks in a uv_idle_t causes a timing issue where
if a handle or request is closed then the class isn't deleted until
uv_close() callbacks are called (which happens after the poll phase).
This results in some destroy callbacks not being called just before the
application exits. So instead switch the destroy callbacks to be called
in a uv_timer_t with the timeout set to zero.

When uv_run() is called with UV_RUN_ONCE the final operation of the
event loop is to process all remaining timers. By setting the timeout to
zero it results in the destroy callbacks being processed after
uv_close() but before uv_run() returned. Processing the destroyed ids
that were previously missed.

Also, process the destroy_ids_list() in a do {} while() loop that makes
sure the vector is empty before returning. Which also makes running
clear() unnecessary.

Fixes: #13262

@addaleax Hope you don't mind that I added the relevant test you included in #13286 (left you as the committer, but altered the commit message)

CI: https://ci.nodejs.org/job/node-test-commit/10272/

addaleax · 2017-06-01T13:34:17Z

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

+  });
+});
+
+process.on('exit', () => assert.strictEqual(destroyTcpWrapCallCount, 2));


Just fyi, the test from #13353 might be better because it uses AsyncResource to trigger the destroy call, so it doesn’t rely on .close() specifics.

Ah thanks. And I can remove the setTimeout() at the bottom of the test.

AndreasMadsen · 2017-06-01T14:21:53Z

src/async-wrap.cc

-    if (ret.IsEmpty()) {
-      ClearFatalExceptionHandlers(env);
-      FatalException(env->isolate(), try_catch);
+  do {


will this not call the destroy hook synchronously if emitDestroy was called from within the destroy hook? We don't want that because destroy hooks should never be called synchronously.

@AndreasMadsen It would act like a nextTick(). The current execution scope would complete before the destroy hook was called.

Yeah, that came to me a few hours after I wrote that. Thanks!

addaleax

Great, thank you a lot!

trevnorris · 2017-06-01T20:45:05Z

@addaleax Thanks for pointing to the other test. Also forgot to include your test for clearImmediate().

CI: https://ci.nodejs.org/job/node-test-commit/10290/

AndreasMadsen

LGTM, thanks for taking care of this.

Calling the destroy callbacks in a uv_idle_t causes a timing issue where if a handle or request is closed then the class isn't deleted until uv_close() callbacks are called (which happens after the poll phase). This results in some destroy callbacks not being called just before the application exits. So instead switch the destroy callbacks to be called in a uv_timer_t with the timeout set to zero. When uv_run() is called with UV_RUN_ONCE the final operation of the event loop is to process all remaining timers. By setting the timeout to zero it results in the destroy callbacks being processed after uv_close() but before uv_run() returned. Processing the destroyed ids that were previously missed. Also, process the destroy_ids_list() in a do {} while() loop that makes sure the vector is empty before returning. Which also makes running clear() unnecessary. Fixes: nodejs#13262 PR-URL: nodejs#13369 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Andreas Madsen <[email protected]>

Verify that the destroy callback for a TCP server is called before exit if it is closed in another destroy callback. Fixes: nodejs#13262 PR-URL: nodejs#13369 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Andreas Madsen <[email protected]>

trevnorris · 2017-06-02T22:23:38Z

Landed in 1e44fd9 and 78b1358

Thanks all!

Calling the destroy callbacks in a uv_idle_t causes a timing issue where if a handle or request is closed then the class isn't deleted until uv_close() callbacks are called (which happens after the poll phase). This results in some destroy callbacks not being called just before the application exits. So instead switch the destroy callbacks to be called in a uv_timer_t with the timeout set to zero. When uv_run() is called with UV_RUN_ONCE the final operation of the event loop is to process all remaining timers. By setting the timeout to zero it results in the destroy callbacks being processed after uv_close() but before uv_run() returned. Processing the destroyed ids that were previously missed. Also, process the destroy_ids_list() in a do {} while() loop that makes sure the vector is empty before returning. Which also makes running clear() unnecessary. Fixes: #13262 PR-URL: #13369 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Andreas Madsen <[email protected]>

Verify that the destroy callback for a TCP server is called before exit if it is closed in another destroy callback. Fixes: #13262 PR-URL: #13369 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Andreas Madsen <[email protected]>

trevnorris requested review from bnoordhuis and addaleax June 1, 2017 12:28

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

trevnorris force-pushed the destroy-on-uv_timer branch from 93436c1 to 87ae33b Compare June 1, 2017 12:29

addaleax approved these changes Jun 1, 2017

View reviewed changes

AndreasMadsen reviewed Jun 1, 2017

View reviewed changes

mscdex added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Jun 1, 2017

trevnorris force-pushed the destroy-on-uv_timer branch from 87ae33b to fe1e23e Compare June 1, 2017 20:41

addaleax approved these changes Jun 1, 2017

View reviewed changes

trevnorris requested a review from AndreasMadsen June 1, 2017 20:45

addaleax mentioned this pull request Jun 1, 2017

async_wrap: remove erroneous destroy list clear() #13353

Closed

3 tasks

AndreasMadsen approved these changes Jun 1, 2017

View reviewed changes

jasnell approved these changes Jun 1, 2017

View reviewed changes

addaleax mentioned this pull request Jun 1, 2017

build,windows: merge test suite groups #13378

Merged

4 tasks

AndreasMadsen mentioned this pull request Jun 2, 2017

async_hooks: run destroy callbacks before normal exit #13286

Closed

3 tasks

trevnorris force-pushed the destroy-on-uv_timer branch from fe1e23e to a127045 Compare June 2, 2017 22:09

trevnorris force-pushed the destroy-on-uv_timer branch from a127045 to 5e4aaee Compare June 2, 2017 22:10

trevnorris and others added 2 commits June 2, 2017 16:21

trevnorris force-pushed the destroy-on-uv_timer branch from 5e4aaee to 78b1358 Compare June 2, 2017 22:22

trevnorris merged commit 78b1358 into nodejs:master Jun 2, 2017

trevnorris added the backported-to-v8.x label Jun 2, 2017

jasnell mentioned this pull request Jun 5, 2017

8.1.0 Proposal #13483

Closed

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

MylesBorins added the dont-land-on-v6.x label Jul 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[async_wrap] call destroy hooks on uv_timer_t #13369

[async_wrap] call destroy hooks on uv_timer_t #13369

trevnorris commented Jun 1, 2017 •

edited

Loading

addaleax Jun 1, 2017

trevnorris Jun 1, 2017

AndreasMadsen Jun 1, 2017

trevnorris Jun 1, 2017

AndreasMadsen Jun 1, 2017

addaleax left a comment

trevnorris commented Jun 1, 2017

AndreasMadsen left a comment

trevnorris commented Jun 2, 2017

[async_wrap] call destroy hooks on uv_timer_t #13369

[async_wrap] call destroy hooks on uv_timer_t #13369

Conversation

trevnorris commented Jun 1, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Summary

addaleax Jun 1, 2017

Choose a reason for hiding this comment

trevnorris Jun 1, 2017

Choose a reason for hiding this comment

AndreasMadsen Jun 1, 2017

Choose a reason for hiding this comment

trevnorris Jun 1, 2017

Choose a reason for hiding this comment

AndreasMadsen Jun 1, 2017

Choose a reason for hiding this comment

addaleax left a comment

Choose a reason for hiding this comment

trevnorris commented Jun 1, 2017

AndreasMadsen left a comment

Choose a reason for hiding this comment

trevnorris commented Jun 2, 2017

trevnorris commented Jun 1, 2017 •

edited

Loading