process: flush stdout/stderr upon process.exit()

[Fishrock123]

WIP Do not Merge

Checklist

• tests and code linting passes
• a test and/or benchmark is included
• documentation is changed or added
• the commit message follows commit guidelines

Proposed fix for #6456

Adapted from https://github.com/kzc/node/commit/29997921800e00a22d9f92d24704a0021be03bbf without exposing a new streams API.

Done on a public branch in case someone else needs to take it over.

Initial CI: https://ci.nodejs.org/job/node-test-pull-request/2650/

[kzc]

without exposing a new streams API.

@Fishrock123 Good idea converting Writable.prototype.flushSync to be private to lib/internal/process.js.

stream._handle.flushSync() is still public accessible though - it only flushes the libuv write queue, not the higher level node stream chunk write queue (a.k.a. bufferedRequest). Which should be fine if documented.

[eljefedelrodeodeljefe]

hmm. I am not the biggest fan of changing process.exit() behaviour entirely without having an alternative that actually calls exit(3) as it does now - even though I don't like that either, but the above feels like obstructing future improvements. Please let's not bake it in.

Also, would it not be possible and maybe better to do it in cpp, since (1) the whole class could't benefit from it and (2) one would maybe be able to do this without constructing a new buffer.
This might also incite actually exposing the state of "being chunked" in for the class.

[kzc]

@eljefedelrodeodeljefe The process.exit() stdio flushing problem can only be fixed at the libuv level. There's no other way. Node programmers have come to expect process.exit() flushes stdout/stderr - even though it was not strictly true historically on all platforms.

Having another function like process._exit() that exits immediately without flushing would probably be useful.

[eljefedelrodeodeljefe]

yes that is my point. In my try for a patch I was moving process.reallyExit() to node_os.cc naming it os.exit() and aliasing it with reallyExit for backwards compat. There is a valid point for os.exit in case of security issues and similar.

Still, voving the flushSync function definition to cpp would be better style, imo.

[addaleax]

@eljefedelrodeodeljefe

I am not the biggest fan of changing process.exit() behaviour entirely without having an alternative that actually calls exit(3) as it does now - even though I don't like that either, but the above feels like obstructing future improvements.

I think it might be helpful here (at least to me) if you could explain what kind of improvements you are referring to and why this PR specifically “feels like obstructing future improvements”.

[Fishrock123]

Still, voving the flushSync function definition to cpp would be better style, imo.

Why? That is potentially clumsy (have you tried writing JS-esq stuff in V8 c++?) and not any more efficient.

but the above feels like obstructing future improvements. Please let's not bake it in

If you mean "if this lands we may not feel like adding a 'graceful-close'", my answer is: ¯_(ツ)_/¯ this has to land regardless.

I'll bring it up at the CTC to see if anyone has critical objections to doing stuff before exit, but I think we are fine so long as we shutdown without calling additional user code after the tick.

Also consider there is a difference between this and, say, http. One has arbitrary third-party networked connections. The other goes to only one spot per stream. The same could be argued for child process (the connection is perhaps singular and not arbitrary), but since that is not a bug I am not going to include it here.

[Fishrock123]

@kzc This appears to pass on windows; is that that just the result of this never having been an issue on windows?

[Fishrock123]

• Times out on armv6 (pi 1): https://ci.nodejs.org/job/node-test-binary-arm/2126/RUN_SUBSET=2,label=pi1-raspbian-wheezy/tapTestReport/test.tap-115/
• Fails on freebsd10-64?: https://ci.nodejs.org/job/node-test-commit-freebsd/2482/nodes=freebsd10-64/tapTestReport/test.tap-832/
• Linter is failing: https://ci.nodejs.org/job/node-test-linter/2496/console

[kzc]

This appears to pass on windows; is that that just the result of this never having been an issue on windows?

I can't say as I don't run Windows. I've asked the same question on these tickets and haven't had a definitive response as to whether stdout and stderr block on Windows in node. If they do block, then great - uv_flush_sync would not need to be implemented on Windows and the patch is pretty much good to go.

Regarding the timeouts for test-stdout-buffer-flush-on-exit.js, it's to be expected that flushing tens of megabytes (or whatever the test calls for) will take some time. It would take an equal amount of time if the program had not called process.exit() and just exited gracefully.

Not sure why freebsd is failing.

Unrelated note: The test test-stdout-buffer-flush-on-exit.js only exercises flushing the low level libuv write queue, not the higher level node chunk write queue. The reason for this is that only a single write is done in the test. To test both the libuv write queue and the node chunk write queue you have to do something like this:

// this program populates the libuv write queue upon first write over 64K
// then will populate the node stream chunk queue for subsequent writes.
for (var i = 1; i <= 1000; ++i) {
  process.stdout.write((i + 
    ': The quick brown fox jumps over the lazy dog.\n').repeat(1500));
}
process.exit(1);

[kzc]

To possibly fix the test timeout on slower platforms consider changing:

[22, 21, 20, 19, 18, 17, 16, 16, 17, 18, 19, 20, 21, 22].forEach((exponent) => {

to something like:

[21, 22].forEach((exponent) => {

node/test/parallel/test-stdout-buffer-flush-on-exit.js

Line 18 in 543c2e0

[22, 21, 20, 19, 18, 17, 16, 16, 17, 18, 19, 20, 21, 22].forEach((exponent) => {

[eljefedelrodeodeljefe]

@Fishrock123 sigh cc @addaleax see...

1. process.exit is wrongfully used in userland and always has been. It should be discouraged, not just fixed as if they should continue like this. That's why I also disagree that is a bug.
2. Okay, we need to fix it, but not by baking it in. Userland should get a possibility to flush synchronously (which they currently don't have easily access too), but also they should pay the price, by doing it their own e.g. call a flush method or set an option theirselves. Also see (1)
3. It's about async io. Every sync io should be made very explicit and be warned about.
4. no language should have a soft process.exit without having a decent philosophy behind it. Soft exits are generally exiting from functions. Sugaring that seems just wrong from a design POV.
   
   Also please note here (1), whereas the use of event emitters and function returns would have been the right way to implement CLIs
5. Moving it to cpp (or alternatively the JS stream classes) seems better, since then every stream can make use of it, not just exit. As pointed out earlier, it's a good and missing feature to detect whether a stream is chunked and (second feature) to flush it.
6. the C-equivalent would look like fflush(stdout);exit(0) or simply return 0 for implicit flushing. Please don't make node more special in API style than it needs to be.
7. My suggestion would be to move process.reallyExit to os.exit if exit(3)-compliance is needed. Not just getting rid of it.

[Fishrock123]

That's why I also disagree that is a bug.

This is a regression from v1.0.2 (And now v6 for TTYs) and I am going to fix it. Period. I am going to fix it whether or not you disagree that it is a bug.

process.exit is wrongfully used in userland and always has been.

Whatever the definition of "wrongly" is, it is no longer relevant. We have regressed in a way that breaks a significant amount of programs regardless.

no language should have a soft process.exit without having a decent philosophy behind it.

What is a "soft" process.exit()? You mean because of beforeExit()? Not in scope of fixing a regression.

For reference's here is man 3 exit:

Before termination, exit() performs the following functions in the order listed:

1. Call the functions registered with the atexit(3) function, in the reverse order of their registration.
2. Flush all open output streams.
3. Close all open streams.
4. Unlink all files created with the tmpfile(3) function.

[eljefedelrodeodeljefe]

Good discussion -.-

[eljefedelrodeodeljefe]

I am not going to interfere any further then.

[jasnell]

I am going to fix it. Period. I am going to fix it whether or not you disagree that it is a bug.

@Fishrock123 ... Just pointing out, this is not a very "consensus seeking" attitude. Obviously feel free to continue working on it, but the tone of your response here is bordering on being downright rude and inappropriately dismissive. If you disagree with someone, there are more constructive and far more polite ways to say so.

The point that I believe @eljefedelrodeodeljefe is making (and I happen to agree with) is that there ought to be an option for exiting without flushing the stdio buffer. Regardless of whether or not you feel flushing stdio by default is appropriate (it is), I consider having the ability to exit immediately without flushing (how process.exit() currently does) to be a perfectly valid use case.

[Fishrock123]

is making (and I happen to agree with) is that there ought to be an option for exiting without flushing the stdio buffer.

That's cool, but even exit(3) flushes (some stuff?) so that's not even possible.

[eljefedelrodeodeljefe]

Maybe some code helps. Until when do you wanna push this forward? I am gonna make a counter proposal, which even isn't that far off. Just don't have the time in next couple of hours.

Not in this case, it doesn't flush our streams. Only what stdout already has. Also it doesn't unwind cpp functions.

[Fishrock123]

Also note: this doesn't change process.abort(). If you absolutely must exit instantly that is your best option even today.

[jasnell]

I believe I was quite clearly referring to the current behavior of not flushing the stdio buffers in node/libuv, as process.exit() currently does. I was not referring to the behavior of exit(3), and process.abort() is not what I am referring to, so allow me to restate if I was not clear:

I believe the current behavior of process.exit() to exit as soon as possible without flushing the node/libuv buffers for stdout and stderr as proposed by this PR is a valid use case and while the default behavior of process.exit() should be to flush those buffers on exit, there ought to be an option for maintaining the current behavior of process.exit().

[Fishrock123]

I believe the current behavior of process.exit() to exit as soon as possible without flushing the node/libuv buffers for stdout and stderr as proposed by this PR is a valid use case

I can't see a reason why it would be, how? When?

Also, this is still a regression so if you want that it needs to be added a separate feature..

Aand if you happen to run on a system that is blocking (e.g. windows as far as I can tell.) you don't even have a choice..

[kzc]

@Fishrock123 Regarding the test test-stdout-buffer-flush-on-exit.js

> 'foo bar baz quux quuz aaa bbb ccc'.repeat(Math.pow(2, 22)).length
138412032

138MB just for one step of the test is too much data. It could be reduced by a factor of 100 considering that node 4.x, 5.x and 6.x only presently outputs around 64KB of data on Linux upon process.exit()

[Fishrock123]

Closing, I guess this isn't the correct patch and there is still a meta issue.