unix: add uv_flush_sync#876
Conversation
Required for nodejs/node#6773
|
also cc @cjihrig & @bnoordhuis |
|
I really don't think this is a solution. The entire libuv model is built around async operations, and you want libuv to add a convenience function because Node breaks that assumption, just for stdout and stderr. I won't fight it to death, but I'm -1. |
|
@saghul How can we do this from the node side? Do you have suggestions? Should we just float this? Should we be doing an async flush and calling exit when that is done? |
|
@Fishrock123 floating is not a solution. What node can do is use |
|
@saghul I suspect may people will object to that. :( |
|
If the previous behavior was to have them block, we can always fallback to that with some strategically placed setBlocking while we come up with a clean solution, right? |
|
@saghul For TTYs, yes, but this problem still persists for pipes. |
| * Returns 0 on success, non-zero on failure. | ||
| */ | ||
| int uv_flush_sync(uv_stream_t* stream) { | ||
| int rc = uv_stream_set_blocking(stream, 1); |
There was a problem hiding this comment.
do not reset blocking to previous value? why?
There was a problem hiding this comment.
We could, but it sounds like that won't help get this anywhere.
This was an example patch cooked up that Node.js may need in some form.
There is no loop at this point prior to exit(). But let's say there is a loop - can the process described be done without invoking callbacks back into node that would prolong (or prevent) the actual exit()? |
There is a loop, it's just not running. Otherwise handles would be dangling. The process described can be done. The trick is to close all handles except those used for stdio before running the loop again. |
:-) Yes, that's the current dilemma.
And all the non-fd-related tasks would have to be unregistered - timer, idle, thread, (dns?) etc. Would also have to make the libuv callbacks no-ops at this point. It all could be done, it's just not trivial. |
I don't disagree :-S I wonder if it wouldn't make sense for Node to use the C stdio family of functions for std{out,err}, and our This way, at the end of the program Node would just I'll try to put together an addon, if I can find the time. |
libc has its own buffer(s) and once full they block. Plus it would make the code path to output to stdout different from sockets, pipes and files. A libuv flush function may not be philosophically in tune with libuv but it's certainly easier to implement than the alternatives. |
|
It's just an idea. There seem to be 2 problems intertwined here: people want all stdout/err to be shown when Node ends, and people want stdout/err not to interleave. Flushing only seems to solve one of them. The other would be solved by doing synchronous writes, which some seem to like / want. |
That's not an argument I personally am very sympathetic to. I agree with @saghul: it's essentially a libuv user problem (i.e., node), not libuv itself. So, one potential solution I see is this:
Order of stdout and stderr can be preserved without undue effort. It wouldn't work reliably when the process is terminated by a signal but that's no different from when we push everything through fwrite(). |
|
Good idea @bnoordhuis! One possible caveat is that |
|
Another solution would be to create a libuv function to retrieve the pending write bytes for a given uv stream and then clear those pending writes from libuv - essentially a transfer of ownership of the data. At that point node user code would block stdout and flush as required. |
|
Cancelling outstanding writes is fraught with issues on Windows, I'm afraid. I have a PR that adds |
|
As for interleave, such fine grained behavior can only be guaranteed to be deterministic when the all file descriptors involved (stdout and stderr) are blocking. |
Could you offer some detail? If libuv is single threaded for all writes and the data hasn't yet been passed to a write function why would this be an issue? |
What do you mean? That's merely a matter of maintaining total ordering.
I'm not sure what you mean by 'write function' but we use IOCP on Windows, which is a (kind of, sort of) thread pool. |
|
When pipes are non-overlapped (which happens when passing them for stdio IIRC) reads and writes happen in a thread pool (sssh, don't tell anyone!), and the only way to cancel all i/o would be to use |
Ordering was never historically maintained for node 4.x, 5.x and 6.x on Linux once the 64KB OS-level write buffer was exceeded for non-blocking tty fds. Mac only buffers 1024 bytes for tty writes to non-blocking fds. I don't know the OS-level write buffer sizes for FreeBSD and Solaris for ttys. Ordering writes only worked on Mac historically in node 4.x/5.x because it was inadvertently blocking stdio writes on that platform.
That explains it. Since we're discussing write order and given the use of IOCP and a thread pool on Windows, is write order between stdout and stderr presently preserved on Windows? |
For non-overlapped pipes I don't think so. Though this was never guaranteed. |
If that's the case then the only platform that truly preserved write order between stdout and stderr was Mac on node 4.x and 5.x. It wasn't a universal thing. |
Correct. That was kind of an "accident". |
That is why I believe that interleave should not be a concern of libuv, nor node for that matter. If such write fidelity between the two streams is desired then stdout and stderr can be made blocking at program start. But the flushing of process.stdout and process.stderr upon process.exit() matter is a different issue. The fds are not guaranteed to be blocking, nor are they guaranteed to be ttys. |
The blocking problem we appear to be able to handle separately, this only needs to be about flushing on exit. |
Perhaps on libuv's end, but not on node's end, as described in this comment by @piscisaureus on historical context: nodejs/node#6816 (comment) |
|
@Fishrock123 I'm fine with node making process.stdout and process.stderr ttys blocking at start up on all platforms. |
|
I'm closing this since there are no plans to land this in libuv and Node already implemented a "workaround" by doing blocking writes. We can refer this issue in the future if we need to revisit. Thanks everyone for the discussion! |
|
Was the piped tty truncation bug in node upon process.exit() also fixed? |
|
I think it was, since all writes are blocking now. On Sep 15, 2016 13:44, "kzc" notifications@github.com wrote:
|
|
Checking... the piped tty truncation bug still exists on UNIX: It still has a known issue test: https://github.com/nodejs/node/blob/master/test/known_issues/test-stdout-buffer-flush-on-exit.js |
|
AFAICT the resolution at this point is that this is a problem Node needs to On Sep 15, 2016 14:15, "kzc" notifications@github.com wrote:
|
|
I don't think the bug can be resolved without libuv support since the data is in-flight. No matter. The Node.JS maintainers are apparently indifferent to tty pipe truncation upon process.exit() or uncaught exception. |
If the data is in-flight that would mean the write request has not completed yet. Waiting for the tty handles to properly close should be sufficient, I think. |
|
By in-flight I meant data was passed to libuv, but not yet written, and not accessible from libuv users like node. The exact case This has all been hashed over before in countless threads, and we're not covering new ground. I was just interested in solving a real world node bug affecting peoples' applications (including my own) - reliably flushing piped tty data upon process.exit() when you cannot return control to the event loop. But the fix was deemed to be "not in the spirit of libuv" and the node bug will remain unfixed. |
Sure, I know you have good intentions. Alas we haven't yet come across a suitable solution on the libuv side, which is consistent with the library :-S |
|
In light of the fact there is no user land workaround for the issue, prioritizing perceived implementation design purity over a pragmatic 10 line patch that fixes such a fundamental I/O bug affecting node users is puzzling to me. Oh well, I tried. |
|
@kzc I can reopen the discussion, but this patch was already deemed not appropriate. There are ways to solve that, but those imply having to run the loop some more, which is a problem for Node. |
|
Every other potential solution is an order of magnitude more complicated to resolve an issue that only effects the flushing of a couple of file descriptors immediately prior to libc exit(). |
|
I don't disagree, but I don't see why we should compromise. The way I see it, this is similar to libc exit vs _exit. If you do exit() then all fds are closed, the streams flushed and everyone is happy. Properly closing the event loop would be the equivalent. This means closing all handles, then waiting for the close callbacks, and finally cleaning up the loop itself. If you exit your program with _exit(), no streams are flushed, which would be equivalent to using uv_stop and breaking from the loop. AFAIK trying to solve the problem this way hasn't been attempted yet, so I guess it's possible it's not an order of magnitude more complicated. Adding a hack will always be quicker, but we don't want to open that door. |
|
The term "hack" is arbitrary. One person's "hack" is another's engineering solution. No one would claim that any code base of this size is perfect in every way. Node.JS and libuv are not ends unto themselves - they exist to solve users' problems. There's a fundamental I/O bug in node that is easily fixed with a trivial patch and Node.JS and libuv are choosing not to do so. No one is stepping up to fix this problem in a way that suits some people's aesthetic sensibilities so a bug affecting real node users will not be fixed. Even if the patch were floated over node's copy of libuv it'd be better than the status quo. I'd be curious to get @rvagg and @Fishrock123's take on this. |
|
@bnoordhuis Regarding your proposed solution in #876 (comment): That doesn’t work because zero-sized writes may succeed in cases where writes carrying data don’t. @saghul Any chance this could get reopened? I see where your opposition is coming from but, like @kzc said, I think the flexibility it offers to users of libuv outweighs the downside of having an edge case API that doesn’t 100 % match libuv’s paradigm, especially in light of the fact that currently this forces users to abandon the advantages that libuv provides. |
The zero-sized write is just to get a writability notification event. It's basically implementing readiness-based I/O on top of libuv's completion-based I/O model. |
|
@bnoordhuis Sorry, I wasn’t clear: The zero-sized write would not block/succeed immediately, even if any other write would block. I actually tried that out in addaleax/node@55ebf86, addaleax/node@23bd72c if you want to take a look. |
7 participants
Required for nodejs/node#6773
@saghul Assuming this has to also be implemented on windows (although node doesn't need it on windows..) what's the windows version of
uv__write? Do I need to switch over the handle type and call multiple methods?I'm also not sure what kind of docs note should go here?