shutdown node in-flight by gireeshpunathil · Pull Request #21283 · nodejs/node · GitHub

An identified use case for embedders is their ability to tear down Node while it is still running (event loop contain pending events)

Here the assumptions are that:

embedders do not wish to resort to JS routines to initiate shutdown
embedders have the Environment handle handy
embedders stop Node through a second thread.

Fixes: #19365
Refs: nodejs/user-feedback#51

Checklist

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

@gireeshpunathil sadly an error occured when I tried to trigger a build :(

Why is this dependent on __POSIX__ or Node being a shared library?

Also: Do we want to enable breaking out of the currently execution JS code? It might be nice to work towards something similar to Worker::Exit()…

@addaleax - thanks for the review.

Why is this dependent on POSIX or Node being a shared library?

On an assumptions that (a) only embedders need this feature, (b) embedders always use or want to use node as a shared library, and (c) the shared library is only available on POSIX platforms.

Reviewing the code, at least (c) is not true, but unsure on (a) and (b). What do you propose?

opening this up on the regular path has the side effect of hosting an extra async handle in the event loop all the time. But your below suggestion may invalidate that I guess?

It might be nice to work towards something similar to Worker::Exit()…

Sure, thanks - I will see that.

@gireeshpunathil b) isn’t true either. If you’re an embedder, at this point you’re most likely compiling a custom version of node yourself and linking it statically (e.g. electron).

I don’t know about a), but then again I’m not sure we need to restrict it to that?

@addaleax - thanks for the guidance, let me go back and do more study on this - to set the semantic straight.

@gireeshpunathil Thinking about it a bit more … right now, the event loop stopping mechanism for workers piggybacks on the message passing mechanism, grep around for e.g. stop_event_loop_. Maybe refactoring that into its own uv_async_t/HandleWrap would be a good start, so that we could reuse it for the main loop?

@addaleax - some gaps in my understanding, please help with fill those:

uv_stop - I see it as just setting a flag which a subsequent loop iteration will take notice of and exit the loop. Is it thread safe? the current design is to call it from the owner thread of the loop, right? In worker case too, it is only invoked by the owner thread. If we are forcing Node to stop in the middle, we don't have a handle to the thread to invoke uv_stop, it is right now running arbitrary code sequence, am I making sense here?
Your suggestion to refactor stop_event_loop_ based mechanism to HandleWrap mechanism: This is how I am seeing it, let me know if we are in sync:

When a worker is created, create and start a HandleWrap object too, in its event loop.
Cache this Handle in a shared space so that main (or any) thread can access this.
When some one wants to stop a worker, just send an interrupt through the handlewrap object.
Convert this mechanism into an API so that main thread can re-use it too.

Please let me know.

What's the status on this one?

@jasnell - some open questions that I will need to find answers. stalled label looks good for now.

I was looking at the possibility of uv_stop semantics to force-close the loop, but realized it is not possible:

The main loop never shuts down while there are active handles in it.
The uv_stop only breaks out of the libuv for the current iteration, with the live handles in tact.
Once it comes out, it resets the stop_flag:
https://github.com/libuv/libuv/blob/8865e72e25fc8aba94da7c9e18c3b5629d288ecc/src/unix/core.c#L386-L387
Back in node::Start, this does not have any effect and the loop continues.

node/src/node.cc

Lines 794 to 797 in 7182aca

    
           more = uv_loop_alive(env.event_loop()); 
        
           if (more) 
        
             continue;

Workers indeed breaks out fully; but leaves the handles open:

node/src/node_worker.cc

Lines 206 to 214 in 7182aca

    
           if (is_stopped()) break; 
        
           uv_run(&loop_, UV_RUN_DEFAULT); 
        
           if (is_stopped()) break; 
        
           platform->DrainTasks(isolate_); 
        
           more = uv_loop_alive(&loop_); 
        
           if (more && !is_stopped()) 
        
             continue;

So in effect:

uv_stop does not causse exit of the main event loop
uv_stop (if modified to exit fully) causes handle leak, if re-enterd by embedders

may be we should revisit uv_walk and uv_xx_close or uv_stop followed by uv_close ? the former was earlier assessed to be unsafe.

@gireeshpunathil What do you mean by handle leak? All handles associated with an Environment are cleaned up when the Environment is released, so that should be okay, right?

@addaleax - o, yes. I see!

So that means the only issue is to gracefully shutdown the main loop. In theory, we can exit if we use a flag that the stopper thread sets, and the main thread sees in the run loop to break out. But may be there is a better way?

@gireeshpunathil I still think refactoring out the approach we currently use for Workers is the best way here (I think I mentioned that above); we use a combination of an uv_async_t, whose handler calls uv_stop(), and isolate->TerminateExecution().

sure @addaleax, will give it a try

Just a note that this is waiting on #26099

The current mechanism of uses two async handles, one owned by the creator of the worker thread to terminate a running worker, and another one employed by the worker to interrupt its creator on its natural termination. The force termination piggybacks on the message- passing mechanism to inform the worker to quiesce. Also there are few flags that represent the other thread's state / request state because certain code path is shared by multiple control flows, and there are certain code path where the async handles may not have come to life. Refactor into a LoopStopper abstraction that exposes routines to install a handle as well as to save a state. Refs: nodejs#21283

The current mechanism of uses two async handles, one owned by the creator of the worker thread to terminate a running worker, and another one employed by the worker to interrupt its creator on its natural termination. The force termination piggybacks on the message- passing mechanism to inform the worker to quiesce. Also there are few flags that represent the other thread's state / request state because certain code path is shared by multiple control flows, and there are certain code path where the async handles may not have come to life. Refactor into an AsyncRequest abstraction that exposes routines to install a handle as well as to save a state. PR-URL: #26099 Refs: #21283 Reviewed-By: Anna Henningsen <anna@addaleax.net>

resuming from stalled state. I would rebase this, but before that would link to have a thought on the logistics:

where do we accommodate the AsyncRequest object, that can be accessed by a foreign thread - Environment? it makes sense as we are talking about tearing the main environment here.
Process ? this also make sense, but may be that object is already an overloaded assortment?
NodePlatform ? but then is there an easy way for the embedder to reach out to it?
/cc @addaleax @nodejs/embedders

@gireeshpunathil I think Environment makes the most sense, yes.

LGTM with @richardlau 's comments.

pre-land CI: https://ci.nodejs.org/job/node-test-pull-request/21584/

pre-land CI: https://ci.nodejs.org/job/node-test-pull-request/21585/

This commit introduces a `node::Stop()` API. An identified use case for embedders is their ability to tear down Node while it is still running (event loop contain pending events) Here the assumptions are that (i) embedders do not wish to resort to JS routines to initiate shutdown (ii) embedders have the Environment handle handy. (iii) embedders stop Node through a second thread. Fixes: nodejs#19365 Refs: nodejs/user-feedback#51 PR-URL: nodejs#21283 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Michael Dawson <Michael_Dawson@ca.ibm.com>

landed as d35af56

This test was broken by d35af56. Refs: nodejs#21283 Fixes: nodejs#26712

This test was broken by d35af56. Refs: #21283 Fixes: #26712 PR-URL: #26713 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

This commit introduces a `node::Stop()` API. An identified use case for embedders is their ability to tear down Node while it is still running (event loop contain pending events) Here the assumptions are that (i) embedders do not wish to resort to JS routines to initiate shutdown (ii) embedders have the Environment handle handy. (iii) embedders stop Node through a second thread. Fixes: nodejs#19365 Refs: nodejs/user-feedback#51 PR-URL: nodejs#21283 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Michael Dawson <Michael_Dawson@ca.ibm.com>

This test was broken by d35af56. Refs: nodejs#21283 Fixes: nodejs#26712 PR-URL: nodejs#26713 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

This commit introduces a `node::Stop()` API. An identified use case for embedders is their ability to tear down Node while it is still running (event loop contain pending events) Here the assumptions are that (i) embedders do not wish to resort to JS routines to initiate shutdown (ii) embedders have the Environment handle handy. (iii) embedders stop Node through a second thread. Fixes: #19365 Refs: nodejs/user-feedback#51 PR-URL: #21283 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Michael Dawson <Michael_Dawson@ca.ibm.com>

This test was broken by d35af56. Refs: #21283 Fixes: #26712 PR-URL: #26713 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Notable changes: * crypto * Allow deriving public from private keys (Tobias Nießen) [#26278](#26278). * events * Added a `once` function to use `EventEmitter` with promises (Matteo Collina) [#26078](#26078). * tty * Added a `hasColors` method to `WriteStream` (Ruben Bridgewater) [#26247](#26247). * Added NO_COLOR and FORCE_COLOR support (Ruben Bridgewater) [#26485](#26485). * v8 * Added `v8.getHeapSnapshot` and `v8.writeHeapSnapshot` to generate snapshots in the format used by tools such as Chrome DevTools (James M Snell) [#26501](#26501). * worker * Added `worker.moveMessagePortToContext`. This enables using MessagePorts in different vm.Contexts, aiding with the isolation that the vm module seeks to provide (Anna Henningsen) [#26497](#26497). * C++ API * `AddPromiseHook` is now deprecated. This API was added to fill an use case that is served by `async_hooks`, since that has `Promise` support (Anna Henningsen) [#26529](#26529). * Added a `Stop` API to shut down Node.js while it is running (Gireesh Punathil) [#21283](#21283). * meta * [Gireesh Punathil](https://github.com/gireeshpunathil) is now a member of the Technical Steering Committee [#26657](#26657). * Added [Yongsheng Zhang](https://github.com/ZYSzys) to collaborators [#26730](#26730). PR-URL: #26949

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jun 12, 2018

gireeshpunathil added shared_lib Issues and PRs related to use of Node.js as a shared library. embedding Issues and PRs related to embedding Node.js in another project. labels Jun 12, 2018

addaleax reviewed Jun 12, 2018

View reviewed changes

Comment thread src/node.cc Outdated

jasnell added the stalled Issues and PRs that are stalled. label Sep 10, 2018

gireeshpunathil mentioned this pull request Feb 14, 2019

worker: refactor thread life cycle management #26099

Closed

4 tasks

gireeshpunathil removed the stalled Issues and PRs that are stalled. label Mar 1, 2019

mhdawson approved these changes Mar 15, 2019

View reviewed changes

gireeshpunathil force-pushed the nodestop branch 2 times, most recently from 4bcdb27 to c495f43 Compare March 16, 2019 06:11

gireeshpunathil force-pushed the nodestop branch from 4549953 to 5a6f004 Compare March 16, 2019 06:15

gireeshpunathil force-pushed the nodestop branch from 5a6f004 to d35af56 Compare March 16, 2019 07:22

gireeshpunathil merged commit d35af56 into nodejs:master Mar 16, 2019

addaleax added a commit to addaleax/node that referenced this pull request Mar 17, 2019

test: fix test-heapdump-worker

0dad72f

This test was broken by d35af56. Refs: nodejs#21283 Fixes: nodejs#26712

This was referenced Mar 17, 2019

test: fix test-heapdump-worker #26713

Closed

Investigate flaky pummel/test-heapdump-worker #26712

Closed

targos mentioned this pull request Mar 27, 2019

v11.13.0 release proposal #26949

Merged

BethGriggs added the dont-land-on-v10.x label Aug 20, 2019

gireeshpunathil mentioned this pull request Dec 3, 2019

src: improve embedder API #30467

Closed

4 tasks

shutdown node in-flight by gireeshpunathil · Pull Request #21283 · nodejs/node · GitHub

Uh oh!

Uh oh!

shutdown node in-flight - #21283

shutdown node in-flight#21283gireeshpunathil merged 1 commit intonodejs:masternodejs/node:masterfrom gireeshpunathil:nodestopgireeshpunathil/node:nodestopCopy head branch name to clipboard

Conversation

gireeshpunathil commented Jun 12, 2018

Checklist

Uh oh!

nodejs-github-bot commented Jun 12, 2018

Uh oh!

addaleax commented Jun 12, 2018

Uh oh!

Uh oh!

gireeshpunathil commented Jun 13, 2018

Uh oh!

addaleax commented Jun 13, 2018

Uh oh!

gireeshpunathil commented Jun 13, 2018

Uh oh!

addaleax commented Jun 13, 2018

Uh oh!

gireeshpunathil commented Jun 15, 2018

Uh oh!

jasnell commented Sep 10, 2018

Uh oh!

gireeshpunathil commented Sep 11, 2018

Uh oh!

gireeshpunathil commented Feb 8, 2019

Uh oh!

addaleax commented Feb 8, 2019

Uh oh!

gireeshpunathil commented Feb 8, 2019

Uh oh!

addaleax commented Feb 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gireeshpunathil commented Feb 8, 2019

Uh oh!

mhdawson commented Feb 28, 2019

Uh oh!

gireeshpunathil commented Mar 1, 2019

Uh oh!

addaleax commented Mar 1, 2019

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

gireeshpunathil commented Mar 16, 2019

Uh oh!

gireeshpunathil commented Mar 16, 2019

Uh oh!

gireeshpunathil commented Mar 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

shutdown node in-flight#21283
gireeshpunathil merged 1 commit into
nodejs:masterfrom
gireeshpunathil:nodestop

addaleax commented Feb 8, 2019 •
edited
Loading