shutdown node in-flight by gireeshpunathil · Pull Request #21283 · nodejs/node (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation91 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

gireeshpunathil

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:

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

Checklist

@nodejs-github-bot

@nodejs-github-bot nodejs-github-bot added the c++

Issues and PRs that require attention from people who are familiar with C++.

label

Jun 12, 2018

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

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

@gireeshpunathil

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

@addaleax

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

@gireeshpunathil

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

@addaleax

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

@gireeshpunathil

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

  1. 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?
  2. 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:

Please let me know.

@jasnell

What's the status on this one?

@gireeshpunathil

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

@gireeshpunathil

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.

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

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

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:

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.

@addaleax

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

@gireeshpunathil

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

@addaleax

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

@gireeshpunathil

@mhdawson

Just a note that this is waiting on #26099

gireeshpunathil added a commit to gireeshpunathil/node that referenced this pull request

Mar 1, 2019

@gireeshpunathil

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

addaleax pushed a commit that referenced this pull request

Mar 1, 2019

@gireeshpunathil @addaleax

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

@gireeshpunathil

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

@addaleax

mhdawson

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gireeshpunathil

@gireeshpunathil

@gireeshpunathil

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

@gireeshpunathil

addaleax added a commit to addaleax/node that referenced this pull request

Mar 17, 2019

@addaleax

This was referenced

Mar 17, 2019

richardlau pushed a commit that referenced this pull request

Mar 18, 2019

@addaleax @richardlau

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

targos pushed a commit to targos/node that referenced this pull request

Mar 27, 2019

@gireeshpunathil @targos

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

targos pushed a commit to targos/node that referenced this pull request

Mar 27, 2019

@addaleax @targos

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

targos pushed a commit that referenced this pull request

Mar 27, 2019

@gireeshpunathil @targos

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

targos pushed a commit that referenced this pull request

Mar 27, 2019

@addaleax @targos

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

targos added a commit that referenced this pull request

Mar 28, 2019

@targos

Notable changes:

PR-URL: #26949

targos added a commit that referenced this pull request

Mar 28, 2019

@targos

Notable changes:

PR-URL: #26949

BethGriggs pushed a commit that referenced this pull request

Apr 5, 2019

@targos @BethGriggs

Notable changes:

PR-URL: #26949

Reviewers

@addaleax addaleax addaleax approved these changes

@richardlau richardlau richardlau approved these changes

@mhdawson mhdawson mhdawson approved these changes

@bnoordhuis bnoordhuis Awaiting requested review from bnoordhuis

@joyeecheung joyeecheung Awaiting requested review from joyeecheung

@jasnell jasnell Awaiting requested review from jasnell

@tniessen tniessen Awaiting requested review from tniessen

Labels

author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

c++

Issues and PRs that require attention from people who are familiar with C++.

embedding

Issues and PRs related to embedding Node.js in another project.

semver-minor

PRs that contain new features and should be released in the next minor version.