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

Ability to shutdown Node in-flight #19365

Closed
gireeshpunathil opened this issue Mar 15, 2018 · 12 comments · Fixed by #21283
Closed

Ability to shutdown Node in-flight #19365

gireeshpunathil opened this issue Mar 15, 2018 · 12 comments · Fixed by #21283
Labels
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.

Comments

@gireeshpunathil
Copy link
Member

  • Version: All
  • Platform: All
  • Subsystem: uv, core

Scope: C++ applications that embed node as a shared library

Issue: Node's life cycle is designed around the event loop method uv_run that assumes exit only when the last handle is drained from the loop, while the embedder may have a larger life cycle in which node workload may be a small subset. For example, the embedder may want to recycle node in response to a shift in workload / configuration, even while the loop is filled with active handles. Current Node / libuv design does not provide such an on-demand exit route.

Proposal: Define a top level API (such as quiesce) that gracefully shuts down the loop and returns from node::Start().

@gibfahn gibfahn added c++ Issues and PRs that require attention from people who are familiar with C++. shared_lib Issues and PRs related to use of Node.js as a shared library. labels Mar 15, 2018
@mhdawson
Copy link
Member

FYI @nodejs/delivery-channels

@mhdawson
Copy link
Member

@yhwang FYI as this is likely one of the use cases we'd want to include in our extended testing for the shared library support.

@addaleax
Copy link
Member

I have written code for Ayo that does basically exactly this. I’ll try to port it over, especially since I’ve been meaning to do that anyway.

@mhdawson
Copy link
Member

@addaleax great to hear :)

@addaleax addaleax added embedding Issues and PRs related to embedding Node.js in another project. and removed shared_lib Issues and PRs related to use of Node.js as a shared library. labels Mar 15, 2018
@addaleax
Copy link
Member

Opened #19377 with most of those commits, would be cool if those who are interested could take a look.

@yhwang
Copy link
Member

yhwang commented Apr 3, 2018

One of the scenario that I tried to handle before is canceling the long-running javascript execution. And it's a tough case because of the resource clean up. For using node.js as shared lib, the mechanism that we provide to shutdown node may also need to handle this case.

@mhdawson
Copy link
Member

Looks like #19377 landed, @gireeshpunathil can you confirm we have what is needed for your original request.

@zeke
Copy link
Contributor

zeke commented May 15, 2018

TIL

To quiesce is to pause or alter a device or application to achieve a consistent state, usually in preparation for a backup or other maintenance. In software applications that modify information stored on disk, this generally involves flushing any outstanding writes; see buffering.

@addaleax
Copy link
Member

addaleax commented May 15, 2018

I think it’s not quite enough. We basically still want something that exposes uv_stop() and set a flag on the Environment which indicates that the event loop should stop, right?

I have something similar in my upcoming Workers PR, so I can try to make this happen.

It might also make sense to put this in a larger picture and ask what other sensible exiting modes Node.js could use. Rather than introducing a new functions, we maybe want something like this? (API bikeshedding encouraged)

  • process.exit({ code, mode: 'immediately' }): Run no further JS code. Basically process.reallyExit().
  • process.exit({ code, mode: 'runExitListeners' }): Like immediately, but also runs process.on('exit') listeners. [Current default]
  • process.exit({ code, mode: 'graceful' }): Does not actually call exit(3). Rather, we terminate JS execution and return from the main loop. [This is what process.exit() currently does in my upcoming Workers PR.]
  • process.exit({ code, mode: 'quiesce' }): Only calls uv_stop() and sets a flag for indicating that we want to return from the main loop, but returns back to JS. Basically graceful without the execution termination.

@addaleax
Copy link
Member

Btw, the big advantage of switching to the graceful variant as the default at some point in the future is that it would allow us to finally solve the issues with async stdio that we’ve been having.

The downside is that that might not play well with addons which don’t expect error handling for execution terminations.

@gireeshpunathil
Copy link
Member Author

  • yes, for shutting down Node in-flight, src: clean up resources on Environment teardown #19377 is not enough, we need a top level C++ API that an embedder can use. src: clean up resources on Environment teardown #19377 paved way for shutting down uv loop and mange the complexity from within, common embedders do not have access to it yet.

  • @addaleax, process.exit variants do not apply for embedders in general IMO, as those embedders don't want the process to exit through Node.js APIs, but just node.(dll|.so) and its events and resources while continue with the embedder's non-javascript workload.

  • agree on the issues with async stdio and necessity for their user-friendly behavior.

@yhwang
Copy link
Member

yhwang commented May 16, 2018

those embedders don't want the process to exit through Node.js APIs, but just node.(dll|.so) and its events and resources while continue with the embedder's non-javascript workload.

+1 . We do need a C++ API to shutdown the Node.js runtime. For now, the node::Start() uses the current thread to run Node.js. Maybe we also need to improve it to run on a dedicated/separated thread. However, this issue is about shutdown but not startup. We can discuss it separately.

And for the process.exit() enhancement, it's nice to have 👍

gireeshpunathil added a commit to gireeshpunathil/node that referenced this issue Mar 16, 2019
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 issue Mar 27, 2019
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 that referenced this issue Mar 27, 2019
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants