src: cancel pending delayed platform tasks on exit #120

addaleax · 2017-10-22T23:01:54Z

Worker threads need an event loop without active libuv handles in
order to shut down. One source of handles that was previously
not accounted for were delayed V8 tasks; these create timers
that would be standing in the way of clearing the event loop.

To solve this, keep track of the scheduled tasks in a list
and close their timer handles before the corresponding isolate/loop
is removed from the platform.

It is not clear from the V8 documentation what the expectation is
with respect to pending background tasks at the end of the
isolate lifetime; however, an alternative approach of executing
these scheduled tasks when flushing them led to an infinite loop
of tasks scheduling each other; so it seems safe to assume that
the behaviour implemented in this patch is at least acceptable.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
[½] tests and/or benchmarks are included (one of the HTTP tests doesn’t work in workers without it)
commit message follows commit guidelines

Affected core subsystem(s)

src/node_platform

Worker threads need an event loop without active libuv handles in order to shut down. One source of handles that was previously not accounted for were delayed V8 tasks; these create timers that would be standing in the way of clearing the event loop. To solve this, keep track of the scheduled tasks in a list and close their timer handles before the corresponding isolate/loop is removed from the platform. It is not clear from the V8 documentation what the expectation is with respect to pending background tasks at the end of the isolate lifetime; however, an alternative approach of executing these scheduled tasks when flushing them led to an infinite loop of tasks scheduling each other; so it seems safe to assume that the behaviour implemented in this patch is at least acceptable.

addaleax · 2017-10-24T21:47:54Z

@ayojs/core Also definitely a bit trickier. I have tried to write a helpful commit message but I know this is not the most well-known area of Node/Ayo :)

Worker threads need an event loop without active libuv handles in order to shut down. One source of handles that was previously not accounted for were delayed V8 tasks; these create timers that would be standing in the way of clearing the event loop. To solve this, keep track of the scheduled tasks in a list and close their timer handles before the corresponding isolate/loop is removed from the platform. It is not clear from the V8 documentation what the expectation is with respect to pending background tasks at the end of the isolate lifetime; however, an alternative approach of executing these scheduled tasks when flushing them led to an infinite loop of tasks scheduling each other; so it seems safe to assume that the behaviour implemented in this patch is at least acceptable. PR-URL: #120 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>

addaleax · 2017-11-02T23:42:46Z

Landed in 9590bcb

Worker threads need an event loop without active libuv handles in order to shut down. One source of handles that was previously not accounted for were delayed V8 tasks; these create timers that would be standing in the way of clearing the event loop. To solve this, keep track of the scheduled tasks in a list and close their timer handles before the corresponding isolate/loop is removed from the platform. It is not clear from the V8 documentation what the expectation is with respect to pending background tasks at the end of the isolate lifetime; however, an alternative approach of executing these scheduled tasks when flushing them led to an infinite loop of tasks scheduling each other; so it seems safe to assume that the behaviour implemented in this patch is at least acceptable. PR-URL: ayojs/ayo#120 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>

Worker threads need an event loop without active libuv handles in order to shut down. One source of handles that was previously not accounted for were delayed V8 tasks; these create timers that would be standing in the way of clearing the event loop. To solve this, keep track of the scheduled tasks in a list and close their timer handles before the corresponding isolate/loop is removed from the platform. It is not clear from the V8 documentation what the expectation is with respect to pending background tasks at the end of the isolate lifetime; however, an alternative approach of executing these scheduled tasks when flushing them led to an infinite loop of tasks scheduling each other; so it seems safe to assume that the behaviour implemented in this patch is at least acceptable. Original-PR-URL: ayojs/ayo#120 Original-Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> PR-URL: #16700 Reviewed-By: James M Snell <jasnell@gmail.com>

Worker threads need an event loop without active libuv handles in order to shut down. One source of handles that was previously not accounted for were delayed V8 tasks; these create timers that would be standing in the way of clearing the event loop. To solve this, keep track of the scheduled tasks in a list and close their timer handles before the corresponding isolate/loop is removed from the platform. It is not clear from the V8 documentation what the expectation is with respect to pending background tasks at the end of the isolate lifetime; however, an alternative approach of executing these scheduled tasks when flushing them led to an infinite loop of tasks scheduling each other; so it seems safe to assume that the behaviour implemented in this patch is at least acceptable. Original-PR-URL: ayojs/ayo#120 Original-Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> PR-URL: nodejs#16700 Reviewed-By: James M Snell <jasnell@gmail.com>

Worker threads need an event loop without active libuv handles in order to shut down. One source of handles that was previously not accounted for were delayed V8 tasks; these create timers that would be standing in the way of clearing the event loop. To solve this, keep track of the scheduled tasks in a list and close their timer handles before the corresponding isolate/loop is removed from the platform. It is not clear from the V8 documentation what the expectation is with respect to pending background tasks at the end of the isolate lifetime; however, an alternative approach of executing these scheduled tasks when flushing them led to an infinite loop of tasks scheduling each other; so it seems safe to assume that the behaviour implemented in this patch is at least acceptable. Backport-PR-URL: #20901 Original-PR-URL: ayojs/ayo#120 Original-Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> PR-URL: #16700 Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax added lib / src worker labels Oct 22, 2017

addaleax mentioned this pull request Oct 22, 2017

test: add more tests for workers #121

Closed

3 tasks

Qard approved these changes Nov 1, 2017

View reviewed changes

addaleax closed this Nov 2, 2017

addaleax deleted the cancel-pending-delayed branch November 2, 2017 23:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: cancel pending delayed platform tasks on exit #120

src: cancel pending delayed platform tasks on exit #120

addaleax commented Oct 22, 2017

addaleax commented Oct 24, 2017

addaleax commented Nov 2, 2017

src: cancel pending delayed platform tasks on exit #120

src: cancel pending delayed platform tasks on exit #120

Conversation

addaleax commented Oct 22, 2017

Checklist

Affected core subsystem(s)

addaleax commented Oct 24, 2017

addaleax commented Nov 2, 2017