A libuv upgrade in Node v6 breaks the popular david package

[mgol]

• Version: v6.0.0-rc.3 (specifically, commit c3cec1e)
• Platform: Darwin mgol-mbpro.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
• Subsystem: libuv

Unfortunately I don't have an isolated test case, I've only seen the david issue in all v6 RCs while it works fine in v5 & I reported it in alanshaw/david#106. I did a git bisect, though and nailed it down to c3cec1e.

[evanlucas]

/cc @saghul

[kzc]

v6.0.0-rc.2
$ david

@mgol It could be a tty issue in libuv 1.9.0.

Can you try running:

$ david | cat

or

$ david > out.txt
$ cat out.txt

[mgol]

david | cat still prints truncated output while piping output to a file and cating the file works fine.

[kzc]

I can reproduce your original truncated david output problem with v6.0.0-pre. However, both commands in my previous comment above work for me on OSX 10.9.5.

[saghul]

I'll take a look. Main suspect: libuv/libuv@387102b

[mgol]

I'm on latest 10.11.4. I tried both in ZSH & Bash, same results every time.

[kzc]

If I apply this patch to master then david | cat fails (output truncated) here as well:

diff --git a/deps/uv/src/unix/tty.c b/deps/uv/src/unix/tty.c
index 32fa37e..e2e5cb5 100644
--- a/deps/uv/src/unix/tty.c
+++ b/deps/uv/src/unix/tty.c
@@ -80,7 +80,7 @@ int uv_tty_init(uv_loop_t* loop, uv_tty_t* tty, int fd, int readable) {
    * different struct file, hence changing its properties doesn't affect
    * other processes.
    */
-  if (type == UV_TTY) {
+  if (0 && type == UV_TTY) {
     /* Reopening a pty in master mode won't work either because the reopened
      * pty will be in slave mode (*BSD) or reopening will allocate a new
      * master/slave pair (Linux). Therefore check if the fd points to a

[kzc]

@saghul Might be an issue with uv_guess_handle not detecting a tty?

[kzc]

scratch that - uv_guess_handle correctly returns UV_TTY.

[kzc]

If this patch is applied to master with libuv 1.9.0 then david output is not truncated on OS X 10.9.5:

diff --git a/deps/uv/src/unix/tty.c b/deps/uv/src/unix/tty.c
index 32fa37e..9179ca7 100644
--- a/deps/uv/src/unix/tty.c
+++ b/deps/uv/src/unix/tty.c
@@ -135,8 +135,10 @@ skip:
   else
     flags |= UV_STREAM_WRITABLE;

+#if 0
   if (!(flags & UV_STREAM_BLOCKING))
     uv__nonblock(fd, 1);
+#endif

   uv__stream_open((uv_stream_t*) tty, fd, flags);
   tty->mode = UV_TTY_MODE_NORMAL;

[kzc]

/cc @Gottox

[kzc]

I don't think the libuv tty PR is the issue. It appears that process.stdout is not being flushed upon process exit. Making stdout blocking as per patch above can make it work, but that's just side stepping the real issue.

If the david script is changed to call process.exit() only upon encountering the process.stdout "drain" event then everything is output correctly with node master 0899ea7.

What is considered to be the correct node way of flushing stdout upon process.exit()? I think this ought to happen automatically without programmer intervention.

[kzc]

Alternate fix - leave node 6.0.0-rc with libuv 1.9.0 as is and simply apply this patch to david:

--- a/david/bin/david.js    2016-04-20 20:49:35.000000000 -0400
+++ b/david/bin/david.js    2016-04-20 21:03:42.000000000 -0400
@@ -325,7 +325,7 @@
       if (getNonWarnDepNames(deps).length ||
           getNonWarnDepNames(devDeps).length ||
           getNonWarnDepNames(optionalDeps).length) {
-        process.exit(1)
+        process.exitCode = 1
       } else {
         // Log feedback if all dependencies are up to date
         console.log(clc.green('All dependencies up to date'))

This has the effect of letting the program exit naturally without abruptly cutting off the flushing of stdout.

[jasnell]

The not flushing on exit is a bit of a long standing issue. It's on my list of things to get figured out soon-ish. In the meantime, it's possible to register an on-exit handler that can flush but you'll need to make sure everything gets done in a single go as there's no ability to nextTick or setImmediate... Node will exit immediately after the on-exit handlers are called. If you have control over when process.exit() is called, you can orchestrate a graceful exit (see #6175 for an example).

[kzc]

See also: https://github.com/cowboy/node-exit

If process.exit is replaced with exit from the module above, it also appears to correctly drain stdio in david when run with node 6.0.0rc + libuv 1.9.0.

In light of these workarounds I think this node ticket can be closed and no need to revert libuv 1.9.0.

[mgol]

The not flushing on exit is a bit of a long standing issue.

Interesting. Why isn't this an issue in Node <6 then?

[kzc]

I have to admit that the current node behavior of process.exit not flushing stdout/stderr in some cases is completely counter-intuitive. In my opinion tty output streams should always be blocking to eliminate the need for such heroic workarounds.

[jasnell]

Not entirely certain but I believe this libuv update included some changes
around non-blocking I/O to stdout. If that's the case, then it makes sense
given that process.exit forces an immediate exit from the event loop.
Anything still pending, including pending I/O is dropped. The only thing
guaranteed to run are the on exit handlers.

I'm investigating ways of allowing Node to make a more graceful exit, but
that work is still pending.
On Apr 21, 2016 1:49 AM, "Michał Gołębiowski" [email protected]
wrote:

The not flushing on exit is a bit of a long standing issue.

Interesting. Why isn't this an issue in Node <6 then?

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#6297 (comment)

[saghul]

Any chance this can be reproduced only with core modules?

[kzc]

I'm sure given enough time it could be isolated into a standalone test case, but a simple print loop is insufficient to reproduce the issue.

The higher level question that should be addressed before any fix is attempted is whether process.exit ought to flush stdout/stderr at all. If so, there are many possible solutions including making writes to stdout/stderr blocking. If not, then there is no work to do.

[jasnell]

The key challenge to making process.exit flush automatically is that the semantics of process.exit() are such that it should force the node process to exit as quickly as possible. I recently proposed adding a gracefulExit() alternative that would allow apps to do additional cleanup (including flushing stdout) but it was pointed out that the same could easily be done by applications already or provided by a userland module. Personally I'd prefer if it were built into core because it is one of those surprising usability things that tends to trip users up.

[kzc]

That's the thing, gracefulExit() was rejected because it was argued it could be done from user land. So there is a logical disconnect with this david bug report - that app is clearly relying on that same undefined behavior. Node documentation for process.exit should explicitly state that stdout and stderr are not guaranteed to be flushed as this is the current behavior and point to an example of the best prescribed Node Way(tm) to achieve flushing of these streams upon process.exit.

[jasnell]

+1. the documentation could indeed be made significant better here. Interested in doing a pull request? ;-) ... if not, maybe someone from @nodejs/documentation could look into it :-)

[kzc]

I honestly do not know what is considered the best node practise to flush stdout/stderr upon process.exit, so I'll pass on the documentation pull request.

But I agree with you that process.exit really ought to flush these streams as users from other languages would reasonably expect that behavior.

[eljefedelrodeodeljefe]

hmm. No expert and I trust you. It wouldn't jump to main though but to right after StartNodeInstance, let v8 dispose and return to main. Though that might be slightly better than exiting directly, still not perfect.

[Qix-]

@bnoordhuis Right, but the suggestion to use longjmp wasn't necessarily playing towards destructors, but rather avoiding a call to exit(), albeit cheaply.

There is a better way to handle this function. That's all I'm getting at.

[kzc]

If you look to C and libc for historical inspiration (not implementation) there's exit(int) which performs atexit() cleanup and flushes all open output streams and there's _exit(int) which quits abruptly without any cleanup or flushing.

Obviously node's process.exit() is more a kin to libc's abrupt _exit(int). It would ideal to also offer a second officially sanctioned form of exit in core node that parallels libc's exit() functionality - much like the third party exit module already does. What it would be called is not terribly important - process.exitPlusPlus() or something else.

[Qix-]

@kzc There is, it's just not being used to flush streams.

process.exit(0); // exit, but with a little bit of cleanup
process.reallyExit(0); // exit(0);

See #6297 (comment)

[kzc]

Apparent consensus is that the present behavior of process.exit() is to remain the same so a different function will have to be created to drain stdout/stderr and exit.

Something logically equivalent to this user code polyfill using the third party exit module that is already known to work:

if (!process.exitGracefully) process.exitGracefully = require('exit');

There's not much code there:

https://github.com/cowboy/node-exit/blob/master/lib/exit.js

[Qix-]

@kzc while that's a perfectly fine temporary solution, this is a simple problem within Node proper and should be fixed :P I don't think it's a consensus that process.exit() isn't changed; it needs to be changed.

[jasnell]

@kzc ... I had a proposed PR that did exactly that but the consensus was that it's something that can be easily handled in userland. Essentially, if you want to exit gracefully while letting any stdio finish, you'll need to make sure that you're not adding anything to the event loop queue so that the process exits out on it's own naturally.

[kzc]

Exiting on its own naturally doesn't work in practise with complicated nested async logic without rewriting a lot of code.

So if something like process.exitGracefully() is not added to node core all I can suggest is that everyone use the third party exit module and close this ticket.

[Qix-]

@jasnell while I agree that's what users should be doing, the fact is that Node is being used as a scripting language and thus is more prone to users using pre-mature exit() calls.

Since exit() itself (at least in modern times) flushes standard FDs we should emulate that in our async environment. It shouldn't need a special function call for it. Users that haven't pushed a lot of output and that don't normally see the bug described here should be completely unaffected.

[jasnell]

@Qix- ... I agree with you :-) ... just need a PR that implements a solution that would be accepted.

[Qix-]

@jasnell Haha okay :) 👍

[eljefedelrodeodeljefe]

@kzc I also see the need to handle this better and have it on my list, or would welcome a decent PR . node-exit's implementations is jut no proper solution. I am also hoping @Qix- would come up with something since your input above was valid.

Can we close this issue or give it a proper name that reflects what the discussion was about though?

[kzc]

node-exit's implementations is jut no proper solution.

@eljefedelrodeodeljefe What would you do differently than node-exit?

https://github.com/cowboy/node-exit/blob/master/lib/exit.js

Looks good to me, and it's known to work. Even supports closing streams other than stderr and stdout. Other than refactoring it to roll it into node proper, another solution would have to perform the same tasks.

[kzc]

@mgol Would you mind renaming this issue to something like "process.exit() ought to flush stdout/stderr"?

[jasnell]

I'd be more inclined to close this particular issue as we have a couple others already in the tracker for this particular issue. The libuv update just brought the issue more to the forefront.

[kzc]

Well, david remains broken in node 6. So not sure why this issue should be closed. Discussion can certainly move to another Issue. Which one do you recommend @jasnell?

[eljefedelrodeodeljefe]

@kzc because their is no action item for libuv e.g. rolling back the change and no action item only for david. The solution will be something around a change of exit behavior, which is not per se broken.

Concerning node-exit: this looks rather like a hack and you probably want to do this at the C or C++ layer, which was proposed already but didn't go forward. Also @jasnell had a solution, which which have added an API rather than fixing an existing one.

reopen if you don't agree, but I think this can be closed in favor for #6456