Repository navigation
A libuv upgrade in Node v6 breaks the popular david package #6297
Description
Activity
david | catstill prints truncated output while piping output to a file and cating the file works fine.I'll take a look. Main suspect: libuv/libuv@387102b
I'm on latest 10.11.4. I tried both in ZSH & Bash, same results every time.
If I apply this patch to master then
david | catfails (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 ascratch that -
uv_guess_handlecorrectly returns UV_TTY.If this patch is applied to master with libuv 1.9.0 then
davidoutput 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;
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
davidscript is changed to callprocess.exit()only upon encountering theprocess.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.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.
50 remaining items
@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.
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 partyexitmodule and close this ticket.@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.@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?
Reacted by Josh Junonnode-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
stderrandstdout. Other than refactoring it to roll it into node proper, another solution would have to perform the same tasks.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.
Reacted by Josh Junon@kzc because their is no action item for
libuve.g. rolling back the change and no action item only fordavid. 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
Unfortunately I don't have an isolated test case, I've only seen the
davidissue in all v6 RCs while it works fine in v5 & I reported it in alanshaw/david#106. I did agit bisect, though and nailed it down to c3cec1e.