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

stdio buffered writes (chunked) issues & process.exit() truncation #6456

Open
eljefedelrodeodeljefe opened this Issue Apr 28, 2016 · 171 comments

Comments

Projects
None yet
@eljefedelrodeodeljefe
Contributor

eljefedelrodeodeljefe commented Apr 28, 2016

If this is currently breaking your program, please use this temporary fix:

[process.stdout, process.stderr].forEach((s) => {
  s && s.isTTY && s._handle && s._handle.setBlocking &&
    s._handle.setBlocking(true)
})

  • Version: v6, (likely all and backportable)
  • Platform: all
  • Subsystem: process

As noted in #6297 async stdio will not be flushed upon immediate process.exit(). This may lay open general deficiencies around C exit() from C++ functions not being properly unwound and is probably not just introduced by latest libuv updates. It should be considered to add flushing, providing graceful exit and/or improving unwinding C++ stacks.

cc @jasnell, @kzc, @Qix-, @bnoordhuis

Issues

Discussion has been already taking place at several places, e.g. #6297, #6456, #6379

Summaries of Proposals

proposals are not exclusive and could lead to semantically unrelated contributions.

  • aid with process.stdout.flush()
  • process.setBlocking(true)
  • node --blocking-stdio
  • longjmp() towards main at exit in C++
  • move parts of process.exit() / process.reallyExit() to new method os.exit()
  • golang panic()- or c++ throw-like stack unwinding

Discussions by Author (with content)


@ChALkeR
I tried to discuss this some time ago at IRC, but postponed it for quite a long time. Also I started the discussion of this in #1741, but I would like to extract the more specific discussion to a separate issue.

I could miss some details, but will try to give a quick overview here.

Several issues here:

  1. Many calls to console.log (e.g. calling it in a loop) could chew up all the memory and die — #1741, #2970, #3171.
  2. console.log has different behavior while printing to a terminal and being redirected to a file. — #1741 (comment).
  3. Output is sometimes truncated — #6297, there were other ones as far as I remember.
  4. The behaviour seems to differ across platforms.

As I understand it — the output has an implicit write buffer (as it's non-blocking) of unlimited size.

One approach to fixing this would be to:

  1. Introduce an explicit cyclic write buffer.
  2. Make writes to that cyclic buffer blocking.
  3. Make writes from the buffer to the actual output non blocking.
  4. When the cyclic buffer reaches it's maximum size (e.g. 10 MiB) — block further writes to the buffer until a corresponding part of it is freed.
  5. On (normal) exit, make sure the buffer is flushed.

For almost all cases, except for the ones that are currently broken, this would behave as a non-blocking buffer (because writes to the buffer are considerably faster than writes from the buffer to file/terminal).

For cases when the data is being piped to the output too quickly and when the output file/terminal does not manage to output it at the same rate — the write would turn into a blocking operation. It would also be blocking at the exit until all the data is written.

Another approach would be to monitor (and limit) the size of data that is contained in the implicit buffer coming from the async queue, and make the operations block when that limit is reached.

@Qix-

This comment has been minimized.

Show comment
Hide comment
@Qix-

Qix- Apr 28, 2016

Perhaps a list of issues this would address and/or close would be helpful to include since this seems to be a sprawling issue with a lot of fragmented discussion.

Qix- commented Apr 28, 2016

Perhaps a list of issues this would address and/or close would be helpful to include since this seems to be a sprawling issue with a lot of fragmented discussion.

@eljefedelrodeodeljefe

This comment has been minimized.

Show comment
Hide comment
@eljefedelrodeodeljefe

eljefedelrodeodeljefe Apr 28, 2016

Contributor

Yes, just a little late in Europe :( keep 'em coming and I add them above.

Contributor

eljefedelrodeodeljefe commented Apr 28, 2016

Yes, just a little late in Europe :( keep 'em coming and I add them above.

@vsemozhetbyt

This comment has been minimized.

Show comment
Hide comment
@vsemozhetbyt

vsemozhetbyt Apr 28, 2016

Member

Also see #6410

Member

vsemozhetbyt commented Apr 28, 2016

Also see #6410

@addaleax addaleax added the process label Apr 28, 2016

@vsemozhetbyt

This comment has been minimized.

Show comment
Hide comment
@vsemozhetbyt

vsemozhetbyt Apr 28, 2016

Member

Considering all the clarification in the #6410, is there also a theoretical possibility that not only several I/O calls to stdout could not make it, but even one simple console.log() before process.exit() could be truncated or discarded?

Member

vsemozhetbyt commented Apr 28, 2016

Considering all the clarification in the #6410, is there also a theoretical possibility that not only several I/O calls to stdout could not make it, but even one simple console.log() before process.exit() could be truncated or discarded?

@Qix-

This comment has been minimized.

Show comment
Hide comment
@Qix-

Qix- Apr 28, 2016

@vsemozhetbyt that is especially correct if I'm understanding your question correctly.

Qix- commented Apr 28, 2016

@vsemozhetbyt that is especially correct if I'm understanding your question correctly.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax
Member

addaleax commented Apr 28, 2016

@vsemozhetbyt If it’s big enough, definitely. See e.g. test/known_issues/test-stdout-buffer-flush-on-exit.js.

@eljefedelrodeodeljefe

This comment has been minimized.

Show comment
Hide comment
@eljefedelrodeodeljefe

eljefedelrodeodeljefe Apr 28, 2016

Contributor

To reproduce you can do

require('crypto').randomBytes(100000000, function(err, buffer) {
  var token = buffer.toString('hex');
  console.log(token);
  process.exit(0)
});

Edit: @addaleax's hint: test does a similar thing. Sorry @addaleax

Contributor

eljefedelrodeodeljefe commented Apr 28, 2016

To reproduce you can do

require('crypto').randomBytes(100000000, function(err, buffer) {
  var token = buffer.toString('hex');
  console.log(token);
  process.exit(0)
});

Edit: @addaleax's hint: test does a similar thing. Sorry @addaleax

@kzc

This comment has been minimized.

Show comment
Hide comment
@kzc

kzc Apr 28, 2016

@vsemozhetbyt This output is truncated with node 6.0.0 on Mac after approx 40 lines:

node -e 'console.log("The quick brown fox jumps.\n".repeat(40000)); process.exit(7);'

node 5.x and earlier output all 40000 lines on Mac.

kzc commented Apr 28, 2016

@vsemozhetbyt This output is truncated with node 6.0.0 on Mac after approx 40 lines:

node -e 'console.log("The quick brown fox jumps.\n".repeat(40000)); process.exit(7);'

node 5.x and earlier output all 40000 lines on Mac.

@vsemozhetbyt

This comment has been minimized.

Show comment
Hide comment
@vsemozhetbyt

vsemozhetbyt Apr 28, 2016

Member

So now if user does not want to reflow the code all one has is to write something like

const err = {name: 'Error', message: 'something wrong'};
throw err;

instead of

console.log('Error: something wrong');
process.exit(1);

and to deal with all the uncontrolled clutter of debug output?

Member

vsemozhetbyt commented Apr 28, 2016

So now if user does not want to reflow the code all one has is to write something like

const err = {name: 'Error', message: 'something wrong'};
throw err;

instead of

console.log('Error: something wrong');
process.exit(1);

and to deal with all the uncontrolled clutter of debug output?

@kzc

This comment has been minimized.

Show comment
Hide comment
@kzc

kzc Apr 28, 2016

throw err;
and to deal with all the uncontrolled clutter of debug output?

For dev code, sure, but uncaught exceptions in production code is not very elegant or professional.

kzc commented Apr 28, 2016

throw err;
and to deal with all the uncontrolled clutter of debug output?

For dev code, sure, but uncaught exceptions in production code is not very elegant or professional.

@kzc

This comment has been minimized.

Show comment
Hide comment
@kzc

kzc Apr 29, 2016

Related: #6379

Also discusses process.stdio.setBlocking(Boolean)

kzc commented Apr 29, 2016

Related: #6379

Also discusses process.stdio.setBlocking(Boolean)

@eljefedelrodeodeljefe

This comment has been minimized.

Show comment
Hide comment
@eljefedelrodeodeljefe

eljefedelrodeodeljefe Apr 29, 2016

Contributor

added @chalkers thread and updated this issue with some summaries and stuff.

Contributor

eljefedelrodeodeljefe commented Apr 29, 2016

added @chalkers thread and updated this issue with some summaries and stuff.

@Qix-

This comment has been minimized.

Show comment
Hide comment
@Qix-

Qix- Apr 29, 2016

@kzc

node 5.x and earlier output all 40000 lines on Mac.

Not sure what you're talking about.

#!/usr/bin/env bash
. ~/.nvm/nvm.sh

uname -a
echo

function do_buffer_test {
    node <<< 'console.log((new Array(40000)).join("Hello! this is a test!\n"));' | wc -l
    node <<< 'console.log((new Array(40000)).join("Hello! this is a test!\n")); process.exit(1)' | wc -l
    node <<< 'console.log((new Array(40000)).join("Hello! this is a test!\n")); process.reallyExit(1)' | wc -l
    node <<< 'console.log((new Array(40000)).join("Hello! this is a test!\n")); process.abort()' | wc -l
    node <<< 'for (var i = 0; i < 40000; i++) console.log("Hello! this is a test!");' | wc -l
    node <<< 'for (var i = 0; i < 40000; i++) console.log("Hello! this is a test!"); process.exit(1)' | wc -l
    node <<< 'for (var i = 0; i < 40000; i++) console.log("Hello! this is a test!"); process.reallyExit(1)' | wc -l
    node <<< 'for (var i = 0; i < 40000; i++) console.log("Hello! this is a test!"); process.abort()' | wc -l
}

nvm install 0.10
do_buffer_test

nvm install 0.12
do_buffer_test

nvm install 1
do_buffer_test

nvm install 2
do_buffer_test

nvm install 3
do_buffer_test

nvm install 4
do_buffer_test

nvm install 5
do_buffer_test

nvm install 6
do_buffer_test
$ ./test-buffers.sh
Darwin JunonBox.local 15.4.0 Darwin Kernel Version 15.4.0: Fri Feb 26 22:08:05 PST 2016; root:xnu-3248.40.184~3/RELEASE_X86_64 x86_64

v0.10.44 is already installed.
Now using node v0.10.44 (npm v2.15.0)
   40000
   40000
   40000
   40000
   40000
   40000
   40000
   40000
v0.12.13 is already installed.
Now using node v0.12.13 (npm v2.15.0)
   40000
   40000
   40000
   40000
   40000
   40000
   40000
   40000
iojs-v1.8.4 is already installed.
Now using io.js v1.8.4 (npm v2.9.0)
   40000
    2849
    2849
    2849
   40000
   40000
   40000
   40000
iojs-v2.5.0 is already installed.
Now using io.js v2.5.0 (npm v2.13.2)
   40000
    2849
    2849
    2849
   40000
   40000
   40000
   40000
iojs-v3.3.1 is already installed.
Now using io.js v3.3.1 (npm v2.14.3)
   40000
    2849
    2849
    2849
   40000
   40000
   40000
   40000
v4.4.3 is already installed.
Now using node v4.4.3 (npm v2.15.1)
   40000
    2849
    2849
    2849
   40000
   40000
   40000
   40000
v5.11.0 is already installed.
Now using node v5.11.0 (npm v3.8.6)
   40000
    2849
    2849
    2849
   40000
   40000
   40000
   40000
v6.0.0 is already installed.
Now using node v6.0.0 (npm v3.8.6)
   40000
    2849
    2849
    2849
   40000
   40000
   40000
   40000
$ ./test-buffers.sh
Linux -snip- 3.18.27 #1 SMP Wed Feb 17 01:14:23 UTC 2016 x86_64 GNU/Linux

######################################################################## 100.0%
Now using node v0.10.44 (npm v2.15.0)
Creating default alias: default -> 0.10 (-> v0.10.44)
40000
40000
40000
40000
40000
40000
40000
40000
######################################################################## 100.0%
Now using node v0.12.13 (npm v2.15.0)
40000
40000
40000
40000
40000
40000
40000
40000
Downloading https://iojs.org/dist/v1.8.4/iojs-v1.8.4-linux-x64.tar.gz...
######################################################################## 100.0%
Now using io.js v1.8.4 (npm v2.9.0)
40000
2849
2849
2849
40000
40000
40000
40000
Downloading https://iojs.org/dist/v2.5.0/iojs-v2.5.0-linux-x64.tar.xz...
######################################################################## 100.0%
Now using io.js v2.5.0 (npm v2.13.2)
40000
2849
2849
2849
40000
40000
40000
40000
Downloading https://iojs.org/dist/v3.3.1/iojs-v3.3.1-linux-x64.tar.xz...
######################################################################## 100.0%
Now using io.js v3.3.1 (npm v2.14.3)
40000
2849
2849
2849
40000
40000
40000
40000
Downloading https://nodejs.org/dist/v4.4.3/node-v4.4.3-linux-x64.tar.xz...
######################################################################## 100.0%
Now using node v4.4.3 (npm v2.15.1)
40000
2849
2849
2849
40000
40000
40000
40000
Downloading https://nodejs.org/dist/v5.11.0/node-v5.11.0-linux-x64.tar.xz...
######################################################################## 100.0%
Now using node v5.11.0 (npm v3.8.6)
40000
2849
2849
2849
40000
40000
40000
40000
Downloading https://nodejs.org/dist/v6.0.0/node-v6.0.0-linux-x64.tar.xz...
######################################################################## 100.0%
Now using node v6.0.0 (npm v3.8.6)
40000
2849
2849
2849
40000
40000
40000
40000

Looks to me whenever io.js forked is when this started happening. Perhaps @indutny can shed some light on the subject.

Qix- commented Apr 29, 2016

@kzc

node 5.x and earlier output all 40000 lines on Mac.

Not sure what you're talking about.

#!/usr/bin/env bash
. ~/.nvm/nvm.sh

uname -a
echo

function do_buffer_test {
    node <<< 'console.log((new Array(40000)).join("Hello! this is a test!\n"));' | wc -l
    node <<< 'console.log((new Array(40000)).join("Hello! this is a test!\n")); process.exit(1)' | wc -l
    node <<< 'console.log((new Array(40000)).join("Hello! this is a test!\n")); process.reallyExit(1)' | wc -l
    node <<< 'console.log((new Array(40000)).join("Hello! this is a test!\n")); process.abort()' | wc -l
    node <<< 'for (var i = 0; i < 40000; i++) console.log("Hello! this is a test!");' | wc -l
    node <<< 'for (var i = 0; i < 40000; i++) console.log("Hello! this is a test!"); process.exit(1)' | wc -l
    node <<< 'for (var i = 0; i < 40000; i++) console.log("Hello! this is a test!"); process.reallyExit(1)' | wc -l
    node <<< 'for (var i = 0; i < 40000; i++) console.log("Hello! this is a test!"); process.abort()' | wc -l
}

nvm install 0.10
do_buffer_test

nvm install 0.12
do_buffer_test

nvm install 1
do_buffer_test

nvm install 2
do_buffer_test

nvm install 3
do_buffer_test

nvm install 4
do_buffer_test

nvm install 5
do_buffer_test

nvm install 6
do_buffer_test
$ ./test-buffers.sh
Darwin JunonBox.local 15.4.0 Darwin Kernel Version 15.4.0: Fri Feb 26 22:08:05 PST 2016; root:xnu-3248.40.184~3/RELEASE_X86_64 x86_64

v0.10.44 is already installed.
Now using node v0.10.44 (npm v2.15.0)
   40000
   40000
   40000
   40000
   40000
   40000
   40000
   40000
v0.12.13 is already installed.
Now using node v0.12.13 (npm v2.15.0)
   40000
   40000
   40000
   40000
   40000
   40000
   40000
   40000
iojs-v1.8.4 is already installed.
Now using io.js v1.8.4 (npm v2.9.0)
   40000
    2849
    2849
    2849
   40000
   40000
   40000
   40000
iojs-v2.5.0 is already installed.
Now using io.js v2.5.0 (npm v2.13.2)
   40000
    2849
    2849
    2849
   40000
   40000
   40000
   40000
iojs-v3.3.1 is already installed.
Now using io.js v3.3.1 (npm v2.14.3)
   40000
    2849
    2849
    2849
   40000
   40000
   40000
   40000
v4.4.3 is already installed.
Now using node v4.4.3 (npm v2.15.1)
   40000
    2849
    2849
    2849
   40000
   40000
   40000
   40000
v5.11.0 is already installed.
Now using node v5.11.0 (npm v3.8.6)
   40000
    2849
    2849
    2849
   40000
   40000
   40000
   40000
v6.0.0 is already installed.
Now using node v6.0.0 (npm v3.8.6)
   40000
    2849
    2849
    2849
   40000
   40000
   40000
   40000
$ ./test-buffers.sh
Linux -snip- 3.18.27 #1 SMP Wed Feb 17 01:14:23 UTC 2016 x86_64 GNU/Linux

######################################################################## 100.0%
Now using node v0.10.44 (npm v2.15.0)
Creating default alias: default -> 0.10 (-> v0.10.44)
40000
40000
40000
40000
40000
40000
40000
40000
######################################################################## 100.0%
Now using node v0.12.13 (npm v2.15.0)
40000
40000
40000
40000
40000
40000
40000
40000
Downloading https://iojs.org/dist/v1.8.4/iojs-v1.8.4-linux-x64.tar.gz...
######################################################################## 100.0%
Now using io.js v1.8.4 (npm v2.9.0)
40000
2849
2849
2849
40000
40000
40000
40000
Downloading https://iojs.org/dist/v2.5.0/iojs-v2.5.0-linux-x64.tar.xz...
######################################################################## 100.0%
Now using io.js v2.5.0 (npm v2.13.2)
40000
2849
2849
2849
40000
40000
40000
40000
Downloading https://iojs.org/dist/v3.3.1/iojs-v3.3.1-linux-x64.tar.xz...
######################################################################## 100.0%
Now using io.js v3.3.1 (npm v2.14.3)
40000
2849
2849
2849
40000
40000
40000
40000
Downloading https://nodejs.org/dist/v4.4.3/node-v4.4.3-linux-x64.tar.xz...
######################################################################## 100.0%
Now using node v4.4.3 (npm v2.15.1)
40000
2849
2849
2849
40000
40000
40000
40000
Downloading https://nodejs.org/dist/v5.11.0/node-v5.11.0-linux-x64.tar.xz...
######################################################################## 100.0%
Now using node v5.11.0 (npm v3.8.6)
40000
2849
2849
2849
40000
40000
40000
40000
Downloading https://nodejs.org/dist/v6.0.0/node-v6.0.0-linux-x64.tar.xz...
######################################################################## 100.0%
Now using node v6.0.0 (npm v3.8.6)
40000
2849
2849
2849
40000
40000
40000
40000

Looks to me whenever io.js forked is when this started happening. Perhaps @indutny can shed some light on the subject.

@eljefedelrodeodeljefe

This comment has been minimized.

Show comment
Hide comment
@eljefedelrodeodeljefe

eljefedelrodeodeljefe Apr 29, 2016

Contributor

Adding two other possibilities.

  • move parts of process.exit() / process.reallyExit() to new method os.exit(), which may fit what actually is happening better
  • golang panic()- or c++ throw-like stack unwinding.
Contributor

eljefedelrodeodeljefe commented Apr 29, 2016

Adding two other possibilities.

  • move parts of process.exit() / process.reallyExit() to new method os.exit(), which may fit what actually is happening better
  • golang panic()- or c++ throw-like stack unwinding.
@kzc

This comment has been minimized.

Show comment
Hide comment
@kzc

kzc Apr 29, 2016

@Qix- @eljefedelrodeodeljefe Please understand that when you pipe the results you are changing the test. It follows a different code path in node and libuv. You have to observe it on the terminal. So in that regard, the test is more difficult to automate.

I am observing this behavior of Mac OS X 10.9.5. The behavior is different on Mac and Linux. Mac stdout appears to have had blocking writes to the tty historically. See #6297 (comment)

kzc commented Apr 29, 2016

@Qix- @eljefedelrodeodeljefe Please understand that when you pipe the results you are changing the test. It follows a different code path in node and libuv. You have to observe it on the terminal. So in that regard, the test is more difficult to automate.

I am observing this behavior of Mac OS X 10.9.5. The behavior is different on Mac and Linux. Mac stdout appears to have had blocking writes to the tty historically. See #6297 (comment)

@kzc

This comment has been minimized.

Show comment
Hide comment
@kzc

kzc Apr 29, 2016

+1 for new function os.exit(Boolean) that drains stdout/stderr upon exit and leave process.exit() as is.

Actually may have to leave process.exit as is because of the prevalence of workarounds to the stdout flushing problem such as node-exit which might break if the behavior of process.exit changes.

kzc commented Apr 29, 2016

+1 for new function os.exit(Boolean) that drains stdout/stderr upon exit and leave process.exit() as is.

Actually may have to leave process.exit as is because of the prevalence of workarounds to the stdout flushing problem such as node-exit which might break if the behavior of process.exit changes.

@saghul

This comment has been minimized.

Show comment
Hide comment
@saghul

saghul Apr 29, 2016

Member

If memory serves right, and by looking at the results posted by @Qix- I think this is where things started to change: libuv/libuv@b197515 Because we started to open writable TTYs in non-blocking mode. Follow the commit trail for reasonin, reverting is not an option.

Member

saghul commented Apr 29, 2016

If memory serves right, and by looking at the results posted by @Qix- I think this is where things started to change: libuv/libuv@b197515 Because we started to open writable TTYs in non-blocking mode. Follow the commit trail for reasonin, reverting is not an option.

@kzc

This comment has been minimized.

Show comment
Hide comment
@kzc

kzc Apr 29, 2016

reverting is not an option.

Yes, and that particular commit also introduced the tty redirection bug in src/unix/tty.c that was fixed in libuv 1.9.0.

kzc commented Apr 29, 2016

reverting is not an option.

Yes, and that particular commit also introduced the tty redirection bug in src/unix/tty.c that was fixed in libuv 1.9.0.

@saghul

This comment has been minimized.

Show comment
Hide comment
@saghul

saghul Apr 29, 2016

Member

@kzc your point being?

Member

saghul commented Apr 29, 2016

@kzc your point being?

@kzc

This comment has been minimized.

Show comment
Hide comment
@kzc

kzc Apr 29, 2016

Just adding weight to reverting is not an option.

kzc commented Apr 29, 2016

Just adding weight to reverting is not an option.

@kzc

This comment has been minimized.

Show comment
Hide comment
@kzc

kzc Apr 29, 2016

@saghul I will add that prior to the tty redirection fix, Mac stdout appeared to be blocking based on my observations with process.exit tests never truncating tty output on Mac. As of that tty redirect fix it is now non-blocking to make it on par with Linux behavior.

kzc commented Apr 29, 2016

@saghul I will add that prior to the tty redirection fix, Mac stdout appeared to be blocking based on my observations with process.exit tests never truncating tty output on Mac. As of that tty redirect fix it is now non-blocking to make it on par with Linux behavior.

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Apr 29, 2016

Member

Can I suggest closing out all other issues with a "discussion continues in #6456" comment?

Member

bnoordhuis commented Apr 29, 2016

Can I suggest closing out all other issues with a "discussion continues in #6456" comment?

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 29, 2016

Member

+1 ... we don't need multiple issues covering the same thing.

Member

jasnell commented Apr 29, 2016

+1 ... we don't need multiple issues covering the same thing.

jasnell added a commit to jasnell/node that referenced this issue Apr 29, 2016

process: add optional timeout to process.exit()
When set, an internal timer will be set that will exit the
process at the given timeout. In the meantime, the registered
listeners for process.on('exitingSoon') will be invoked and
passed a callback to be called when the handler is ready for
the process to exit. The process will exit either when the
internal timer fires or all the callbacks are called, whichever
comes first.

This is an attempt to deal more intelligently with resource
cleanup and async op completion on exit

(see nodejs#6456).

@jasnell jasnell referenced this issue Apr 29, 2016

Closed

process: add process.exitSoon() #6477

0 of 4 tasks complete
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 29, 2016

Member

See: #6477

Member

jasnell commented Apr 29, 2016

See: #6477

@chalkers

This comment has been minimized.

Show comment
Hide comment
@chalkers

chalkers commented May 1, 2016

@eljefedelrodeodeljefe you got the wrong @ChALkeR :)

@bcoe

This comment has been minimized.

Show comment
Hide comment
@bcoe

bcoe May 10, 2016

Member

This is causing some fairly wonky behavior with yargs, two questions:

  1. should we be classifying this as a bug (the new flushing behavior seems unintuitive), if so is there a separate tracking ticket I should be following?
  2. is there a recommended workaround, or should I hold my horses for a patch.

Between commander, yargs, and optimist (all of which now exhibit broken behavior) this is going to be hitting a lot of people (about 1,600,000 installs a day).

Member

bcoe commented May 10, 2016

This is causing some fairly wonky behavior with yargs, two questions:

  1. should we be classifying this as a bug (the new flushing behavior seems unintuitive), if so is there a separate tracking ticket I should be following?
  2. is there a recommended workaround, or should I hold my horses for a patch.

Between commander, yargs, and optimist (all of which now exhibit broken behavior) this is going to be hitting a lot of people (about 1,600,000 installs a day).

@kzc

This comment has been minimized.

Show comment
Hide comment
@kzc

kzc May 10, 2016

@bcoe You won't find consensus on the "process.exit() not flushing stdio" issue because many node devs don't think it's a problem.

If you must use process.exit() the only known workaround is have stdout and stderr block at application start:

process.stdout._handle.setBlocking(true);
process.stderr._handle.setBlocking(true);

Cue the "it's not supported" and "that's not the node way" rebuttals...

kzc commented May 10, 2016

@bcoe You won't find consensus on the "process.exit() not flushing stdio" issue because many node devs don't think it's a problem.

If you must use process.exit() the only known workaround is have stdout and stderr block at application start:

process.stdout._handle.setBlocking(true);
process.stderr._handle.setBlocking(true);

Cue the "it's not supported" and "that's not the node way" rebuttals...

@eljefedelrodeodeljefe

This comment has been minimized.

Show comment
Hide comment
@eljefedelrodeodeljefe

eljefedelrodeodeljefe May 10, 2016

Contributor

@bcoe this is the tracking ticket. If possible don't do ._handle.setBlocking, since this will affect the whole process users start with yargs. There will definitely be no revert on the libuv side and this shouldn't be considered a bug. No-one has come up with a decent workaround, around flushing and properly unwinding on exit. I think it's gonna take a while.

There are workarounds however that would be immediately possible, but would require refactoring, namely avoiding the programmtic use of exit handlers. Where exactly in the code base is that a problem?

Contributor

eljefedelrodeodeljefe commented May 10, 2016

@bcoe this is the tracking ticket. If possible don't do ._handle.setBlocking, since this will affect the whole process users start with yargs. There will definitely be no revert on the libuv side and this shouldn't be considered a bug. No-one has come up with a decent workaround, around flushing and properly unwinding on exit. I think it's gonna take a while.

There are workarounds however that would be immediately possible, but would require refactoring, namely avoiding the programmtic use of exit handlers. Where exactly in the code base is that a problem?

@bcoe

This comment has been minimized.

Show comment
Hide comment
@bcoe

bcoe May 10, 2016

Member

@eljefedelrodeodeljefe the problem is less with yargs itself, and more with the consuming library. With optimist, yargs, and (I would guess) commander, there are commands that force an exit preventing the program consuming the library from attempting to handle the parser output:

var argv = require('yargs')(['--help'])
  .help()
  .argv

console.log('we should never get here');

The above code would never hit the console.log line, and would process.exit(0);. Perhaps this would be an acceptable workaround?

if (shouldExit) {
process.stdout._handle.setBlocking(true);
process.stderr._handle.setBlocking(true);
console.log(yargs.help());
process.exit(0);
}

avoid setting stdout and stderr to blocking until we already know we are about to exit?

Member

bcoe commented May 10, 2016

@eljefedelrodeodeljefe the problem is less with yargs itself, and more with the consuming library. With optimist, yargs, and (I would guess) commander, there are commands that force an exit preventing the program consuming the library from attempting to handle the parser output:

var argv = require('yargs')(['--help'])
  .help()
  .argv

console.log('we should never get here');

The above code would never hit the console.log line, and would process.exit(0);. Perhaps this would be an acceptable workaround?

if (shouldExit) {
process.stdout._handle.setBlocking(true);
process.stderr._handle.setBlocking(true);
console.log(yargs.help());
process.exit(0);
}

avoid setting stdout and stderr to blocking until we already know we are about to exit?

@eljefedelrodeodeljefe

This comment has been minimized.

Show comment
Hide comment
@eljefedelrodeodeljefe

eljefedelrodeodeljefe May 10, 2016

Contributor

dragging in @bnoordhuis. Have you worked on this in the meantime? Would that be an acceptable hotfix until we come up with a proper solution?

Contributor

eljefedelrodeodeljefe commented May 10, 2016

dragging in @bnoordhuis. Have you worked on this in the meantime? Would that be an acceptable hotfix until we come up with a proper solution?

@sindresorhus

This comment has been minimized.

Show comment
Hide comment
@sindresorhus

sindresorhus May 10, 2016

Using process.exit() is common convention in CLI tools. The change in Node.js 6 has pretty much broken everything CLI related... I use process.exit() in meow which a lot of packages depend on (5,312,249 downloads in the last month).

process.exit() will be especially useful when ES2015 modules comes to Node.js, as we can then no longer return in the top-scope, so short-circuiting will be effectively impossible, without a nesting mess.

sindresorhus commented May 10, 2016

Using process.exit() is common convention in CLI tools. The change in Node.js 6 has pretty much broken everything CLI related... I use process.exit() in meow which a lot of packages depend on (5,312,249 downloads in the last month).

process.exit() will be especially useful when ES2015 modules comes to Node.js, as we can then no longer return in the top-scope, so short-circuiting will be effectively impossible, without a nesting mess.

cjihrig added a commit that referenced this issue Aug 11, 2016

tty: set the handle to blocking mode
Refs: #1771
Refs: #6456
Refs: #6773
Refs: #7743
PR-URL: #6816
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

honzajavorek added a commit to apiaryio/dredd that referenced this issue Oct 4, 2016

@Qix-

This comment has been minimized.

Show comment
Hide comment
@Qix-

Qix- Feb 13, 2017

This is probably a dumb question, but have we tried simply switching the system call exit() to _exit()?

This is from vfork()'s manpage but it made me think of this issue.

Be careful, also, to call _exit rather than exit if you can't execve, since exit will flush and close standard I/O channels, and thereby mess up the parent processes standard I/O data structures. (Even with fork it is wrong to call exit since buffered data would then be flushed twice.)

I haven't tried it and have no idea if they're related. I'm not sure if we were ever sure it was simply some async events being dropped from the queue in libuv or not.

If that's still the case, then walking the libuv queue might still be the best possible solution.

Qix- commented Feb 13, 2017

This is probably a dumb question, but have we tried simply switching the system call exit() to _exit()?

This is from vfork()'s manpage but it made me think of this issue.

Be careful, also, to call _exit rather than exit if you can't execve, since exit will flush and close standard I/O channels, and thereby mess up the parent processes standard I/O data structures. (Even with fork it is wrong to call exit since buffered data would then be flushed twice.)

I haven't tried it and have no idea if they're related. I'm not sure if we were ever sure it was simply some async events being dropped from the queue in libuv or not.

If that's still the case, then walking the libuv queue might still be the best possible solution.

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Feb 13, 2017

Member

That's unrelated to what is happening in node, except in loosest philosophical sense.

They refer to C's FILE objects which have their own buffering, and you can choose to flush those buffers on exit() or to call _exit() and not flush the FILE objects.

In node's case, process.exit() is like _exit(), and nodes exited because the loop has nothing to do is the equivalent of exit(), which is what confuses people - they think they can just call process.exit() and their data will be written, their HTTP requests will complete, etc, etc, but that's a case of "not thinking asyncly".

Member

sam-github commented Feb 13, 2017

That's unrelated to what is happening in node, except in loosest philosophical sense.

They refer to C's FILE objects which have their own buffering, and you can choose to flush those buffers on exit() or to call _exit() and not flush the FILE objects.

In node's case, process.exit() is like _exit(), and nodes exited because the loop has nothing to do is the equivalent of exit(), which is what confuses people - they think they can just call process.exit() and their data will be written, their HTTP requests will complete, etc, etc, but that's a case of "not thinking asyncly".

@addaleax addaleax referenced this issue May 11, 2017

Closed

process: always make stdio blocking #12970

4 of 4 tasks complete

addaleax added a commit to addaleax/node that referenced this issue May 11, 2017

process: always make stdio blocking
Always make stdio blocking. There have only been a few exceptions to
this rule and I can’t seem to find any reason for those that outweighs
the benefits.

Fixes: nodejs#6456
Fixes: nodejs#12921
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott May 28, 2017

Member

@Fishrock123 I'm removing this note from the post at the top:

CTC Note (Fishrock123): We are working on resolving this. Unfortunately it's not the most trivial thing to investigate or fix well.

I don't think anyone is actually working on this actively at the current time. If that's wrong, feel free to put the note back, of course.

Member

Trott commented May 28, 2017

@Fishrock123 I'm removing this note from the post at the top:

CTC Note (Fishrock123): We are working on resolving this. Unfortunately it's not the most trivial thing to investigate or fix well.

I don't think anyone is actually working on this actively at the current time. If that's wrong, feel free to put the note back, of course.

@pmuens pmuens referenced this issue Jul 6, 2017

Merged

Fix dev dependency exclusion bug #3889

5 of 5 tasks complete

kachkaev added a commit to kachkaev/run-elm that referenced this issue Feb 16, 2018

jfairbank added a commit to jfairbank/run-elm that referenced this issue Feb 21, 2018

Fix stdout truncation (#8)
* Support long output from Elm

Related to nodejs/node#6456

* Update CHANGELOG

* Configure tests with test-config.js instead of input.txt and output.txt
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Mar 4, 2018

Member

Is this still an issue? (If so, is anyone working on it? I'll add a help wanted label at least if that's the case.) Should this remain open?

Member

Trott commented Mar 4, 2018

Is this still an issue? (If so, is anyone working on it? I'll add a help wanted label at least if that's the case.) Should this remain open?

@isiahmeadows

This comment has been minimized.

Show comment
Hide comment
@isiahmeadows

isiahmeadows Mar 4, 2018

@Trott Based on some of these recent issues/commits linking to here, I suspect it still is an issue. (I've long had to move on to fix async writes, since it was causing tests to fail.)

isiahmeadows commented Mar 4, 2018

@Trott Based on some of these recent issues/commits linking to here, I suspect it still is an issue. (I've long had to move on to fix async writes, since it was causing tests to fail.)

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Jul 16, 2018

Contributor

Is this still an issue?

Yes, I can confirm 10.6.0 still shows this issue. Simple repro:

$ node -p 'process.stdout.write("x".repeat(5e6)); process.exit()' | wc -c
65536
Contributor

silverwind commented Jul 16, 2018

Is this still an issue?

Yes, I can confirm 10.6.0 still shows this issue. Simple repro:

$ node -p 'process.stdout.write("x".repeat(5e6)); process.exit()' | wc -c
65536

xzyfer added a commit to sass/node-sass that referenced this issue Jul 19, 2018

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