Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: don't call uv_run() after 'exit' event #12344

Closed
wants to merge 1 commit into from

Conversation

@bnoordhuis
Copy link
Member

commented Apr 11, 2017

This breaks parallel/test-async-wrap-throw-from-callback.

Suggestions on how to keep the changes from #9753 working welcome.

@trevnorris

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2017

Note: The test parallel/test-async-wrap-throw-from-callback has been deleted in my PR #11883 and this change doesn't affect any other tests.

src: don't call uv_run() after 'exit' event
It makes timers and other libuv handles fire intermittently after the
'exit' event, contrary to what the documentation states.

This change breaks parallel/test-async-wrap-throw-from-callback which I
pragmatically resolved by removing the test.  In all seriousness, it is
scheduled for removal anyway in the upcoming async_wrap revamp so there
isn't much point in sinking a lot of time in it.

@bnoordhuis bnoordhuis force-pushed the bnoordhuis:fix-12322-break-9753 branch to 2d4f4eb Apr 13, 2017

@bnoordhuis bnoordhuis changed the title src: don't call uv_run() after 'exit' event [DO NOT LAND] src: don't call uv_run() after 'exit' event Apr 13, 2017

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2017

Fixed (what's in a name?) by removing the test. New CI: https://ci.nodejs.org/job/node-test-pull-request/7374/

@refack refack force-pushed the nodejs:master branch to fbe946b Apr 14, 2017

@bnoordhuis bnoordhuis closed this Apr 19, 2017

bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Apr 19, 2017
src: don't call uv_run() after 'exit' event
It makes timers and other libuv handles fire intermittently after the
'exit' event, contrary to what the documentation states.

Regression introduced in commit aac79df ("src: use stack-allocated
Environment instances") from June last year that made the
`while (handle_cleanup_waiting_ != 0) uv_run(event_loop(), UV_RUN_ONCE)`
loop run unconditionally on exit because it merged CleanupHandles() into
the Environment destructor.

This change breaks parallel/test-async-wrap-throw-from-callback because
the async_wrap idle handle is no longer cleaned up, which I resolved
pragmatically by removing the test.

In all seriousness, it is being removed in the upcoming async_wrap
revamp - it doesn't make sense to sink a lot of time in it now.

Fixes: nodejs#12322
PR-URL: nodejs#12344
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@bnoordhuis bnoordhuis referenced this pull request Apr 27, 2017
2 of 2 tasks complete
danbev added a commit to danbev/node that referenced this pull request May 2, 2017
src: don't call uv_run() after 'exit' event
It makes timers and other libuv handles fire intermittently after the
'exit' event, contrary to what the documentation states.

Regression introduced in commit aac79df ("src: use stack-allocated
Environment instances") from June last year that made the
`while (handle_cleanup_waiting_ != 0) uv_run(event_loop(), UV_RUN_ONCE)`
loop run unconditionally on exit because it merged CleanupHandles() into
the Environment destructor.

This change breaks parallel/test-async-wrap-throw-from-callback because
the async_wrap idle handle is no longer cleaned up, which I resolved
pragmatically by removing the test.

In all seriousness, it is being removed in the upcoming async_wrap
revamp - it doesn't make sense to sink a lot of time in it now.

Fixes: nodejs#12322
PR-URL: nodejs#12344
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell jasnell referenced this pull request May 11, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
@gibfahn

This comment has been minimized.

Copy link
Member

commented Jun 18, 2017

@bnoordhuis looks like this was landed in 5ef6000

@gibfahn

This comment has been minimized.

Copy link
Member

commented Jun 18, 2017

I'm assuming/guessing this doesn't need to land on v6.x. Please raise a backport PR if that's not correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.