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

doc: expand documentation for process.exit() #6410

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@jasnell
Member

jasnell commented Apr 27, 2016

Checklist
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

doc (process)

Description of change

The fact that process.exit() interrupts pending async operations such as non-blocking i/o is becoming a bit more pronounced with the recent libuv update. This commit expands the documentation for process.exit() to explain clearly how it affects async operations.

@ChALkeR

This comment has been minimized.

Show comment
Hide comment
@ChALkeR

ChALkeR Apr 27, 2016

Member

LGTM

Member

ChALkeR commented Apr 27, 2016

LGTM

@cjihrig

View changes

Show outdated Hide outdated doc/api/process.md
@cjihrig

View changes

Show outdated Hide outdated doc/api/process.md
@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Apr 27, 2016

Contributor

LGTM with a couple comments.

Contributor

cjihrig commented Apr 27, 2016

LGTM with a couple comments.

@Fishrock123

View changes

Show outdated Hide outdated doc/api/process.md
@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Apr 28, 2016

Member

Additional note that I'm not sure fits in: It's actually "safer" to exit the process by throwing.

Member

Fishrock123 commented Apr 28, 2016

Additional note that I'm not sure fits in: It's actually "safer" to exit the process by throwing.

jasnell added some commits Apr 27, 2016

doc: expand documentation for process.exit()
The fact that process.exit() interrupts pending async operations
such as non-blocking i/o is becoming a bit more pronounced with
the recent libuv update. This commit expands the documentation
for `process.exit()` to explain clearly how it affects async
operations.
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 28, 2016

Member

@cjihrig @Fishrock123 ... updated to address nits

Member

jasnell commented Apr 28, 2016

@cjihrig @Fishrock123 ... updated to address nits

@eljefedelrodeodeljefe

This comment has been minimized.

Show comment
Hide comment
@eljefedelrodeodeljefe

eljefedelrodeodeljefe Apr 28, 2016

Contributor

LGTM, very good. I hope this will clarified through API rather in the future, as per linked discussions.

Contributor

eljefedelrodeodeljefe commented Apr 28, 2016

LGTM, very good. I hope this will clarified through API rather in the future, as per linked discussions.

@Qix-

This comment has been minimized.

Show comment
Hide comment
@Qix-

Qix- Apr 28, 2016

Lgtm, sharing the same sentiments as @eljefedelrodeodeljefe.

Qix- commented Apr 28, 2016

Lgtm, sharing the same sentiments as @eljefedelrodeodeljefe.

@kzc

This comment has been minimized.

Show comment
Hide comment
@kzc

kzc Apr 28, 2016

Should add a description of how to correctly flush/drain stdout/stderr if process.exit() must be used.

kzc commented Apr 28, 2016

Should add a description of how to correctly flush/drain stdout/stderr if process.exit() must be used.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 29, 2016

Member

@kzc ... do you recommend a particular approach for that example?

Member

jasnell commented Apr 29, 2016

@kzc ... do you recommend a particular approach for that example?

@kzc

This comment has been minimized.

Show comment
Hide comment
@kzc

kzc Apr 29, 2016

@kzc ... do you recommend a particular approach for that example?

@jasnell I'd have to recommend the use of node-exit as it is widely used and I've tested it to work on node versions 0.10.x - 6.0.0 on Mac and Linux. I assume it works on Windows as that seems to be why it was specifically created. (Edit: no longer recommended - see #6410 (comment))

https://www.npmjs.com/package/exit

Some might argue that it's less than ideal, but it was created to fill a need. People want to call process.exit() and expect to have stdout/stderr flushed - rightly or wrongly.

kzc commented Apr 29, 2016

@kzc ... do you recommend a particular approach for that example?

@jasnell I'd have to recommend the use of node-exit as it is widely used and I've tested it to work on node versions 0.10.x - 6.0.0 on Mac and Linux. I assume it works on Windows as that seems to be why it was specifically created. (Edit: no longer recommended - see #6410 (comment))

https://www.npmjs.com/package/exit

Some might argue that it's less than ideal, but it was created to fill a need. People want to call process.exit() and expect to have stdout/stderr flushed - rightly or wrongly.

@eljefedelrodeodeljefe

This comment has been minimized.

Show comment
Hide comment
@eljefedelrodeodeljefe

eljefedelrodeodeljefe Apr 29, 2016

Contributor

@kzc sorry, but we cannot do this. Hints to userland packages are really rare. Also the way they do it is not advisable. Please just wait for resolutions around process.exit().

Since you haven't brought up an example, I would say this PR is good to merge.

Contributor

eljefedelrodeodeljefe commented Apr 29, 2016

@kzc sorry, but we cannot do this. Hints to userland packages are really rare. Also the way they do it is not advisable. Please just wait for resolutions around process.exit().

Since you haven't brought up an example, I would say this PR is good to merge.

@kzc

This comment has been minimized.

Show comment
Hide comment
@kzc

kzc Apr 29, 2016

Then make an equivalent example not citing this third party user land package that drains stdout and stderr. This is a legitimate concern and is the obvious question people ask when they use process.exit.

Edit: There's a reason why node-exit has had 3.5 million downloads in the last month. To workaround this very issue.

kzc commented Apr 29, 2016

Then make an equivalent example not citing this third party user land package that drains stdout and stderr. This is a legitimate concern and is the obvious question people ask when they use process.exit.

Edit: There's a reason why node-exit has had 3.5 million downloads in the last month. To workaround this very issue.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 29, 2016

Member

@kzc ... my preference would be to go ahead and get this change landed as is then add an example of stdout/stderr draining later once we're a bit more settled on the right approach. Fair?

Member

jasnell commented Apr 29, 2016

@kzc ... my preference would be to go ahead and get this change landed as is then add an example of stdout/stderr draining later once we're a bit more settled on the right approach. Fair?

@kzc

This comment has been minimized.

Show comment
Hide comment
@kzc

kzc commented Apr 29, 2016

@jasnell Cool.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 29, 2016

Member

@ChALkeR @cjihrig ... still LGTY?

Member

jasnell commented Apr 29, 2016

@ChALkeR @cjihrig ... still LGTY?

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Apr 29, 2016

Contributor

Yep.

Contributor

cjihrig commented Apr 29, 2016

Yep.

jasnell added a commit that referenced this pull request Apr 29, 2016

doc: expand documentation for process.exit()
The fact that process.exit() interrupts pending async operations
such as non-blocking i/o is becoming a bit more pronounced with
the recent libuv update. This commit expands the documentation
for `process.exit()` to explain clearly how it affects async
operations.

PR-URL: #6410
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 29, 2016

Member

Landed in c2bbfe2

Member

jasnell commented Apr 29, 2016

Landed in c2bbfe2

@kzc

This comment has been minimized.

Show comment
Hide comment
@kzc

kzc Apr 30, 2016

I just studied the implementation of the third party node-exit module. Contrary to my prior remarks, it is not advisable for it to be used as a replacement for process.exit to flush stdout and stderr.

It only does partial stdout/stderr flushing at the best of times, and at worse it simply disables writes to stdout/stderr, and then it returns and allows program execution to continue - which can have bad consequences. Because node-exit happens to stop output to stdio and it installs an on "exit" handler which calls process.exit() with the correct exit code, it had me fooled in initial testing.

I do not believe it is possible to fully flush any stream synchronously immediately prior to calling process.exit() as node is currently implemented.

kzc commented Apr 30, 2016

I just studied the implementation of the third party node-exit module. Contrary to my prior remarks, it is not advisable for it to be used as a replacement for process.exit to flush stdout and stderr.

It only does partial stdout/stderr flushing at the best of times, and at worse it simply disables writes to stdout/stderr, and then it returns and allows program execution to continue - which can have bad consequences. Because node-exit happens to stop output to stdio and it installs an on "exit" handler which calls process.exit() with the correct exit code, it had me fooled in initial testing.

I do not believe it is possible to fully flush any stream synchronously immediately prior to calling process.exit() as node is currently implemented.

Fishrock123 added a commit that referenced this pull request May 4, 2016

doc: expand documentation for process.exit()
The fact that process.exit() interrupts pending async operations
such as non-blocking i/o is becoming a bit more pronounced with
the recent libuv update. This commit expands the documentation
for `process.exit()` to explain clearly how it affects async
operations.

PR-URL: #6410
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com>

joelostrowski added a commit to joelostrowski/node that referenced this pull request May 4, 2016

doc: expand documentation for process.exit()
The fact that process.exit() interrupts pending async operations
such as non-blocking i/o is becoming a bit more pronounced with
the recent libuv update. This commit expands the documentation
for `process.exit()` to explain clearly how it affects async
operations.

PR-URL: nodejs#6410
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins May 18, 2016

Member

This does not land cleanly on v4.x

if someone would like to manually backport let me know

Member

MylesBorins commented May 18, 2016

This does not land cleanly on v4.x

if someone would like to manually backport let me know

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins
Member

MylesBorins commented Jun 1, 2016

ping @jasnell

@kzc

This comment has been minimized.

Show comment
Hide comment
@kzc

kzc Jun 1, 2016

In light of #6867 this sentence in the doc PR is incorrect:

If it is necessary to terminate the Node.js process due to an error condition, throwing an uncaught error and allowing the process to terminate accordingly is safer than calling process.exit().

kzc commented Jun 1, 2016

In light of #6867 this sentence in the doc PR is incorrect:

If it is necessary to terminate the Node.js process due to an error condition, throwing an uncaught error and allowing the process to terminate accordingly is safer than calling process.exit().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment