Skip to content
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

documentation on Stream.write misses that callback does not ensure that data written to process.stdout/.stderr is flushed #3670

Closed
minesworld opened this issue Nov 5, 2015 · 12 comments
Labels
doc Issues and PRs related to the documentations. process Issues and PRs related to the process subsystem.

Comments

@minesworld
Copy link

 var empty = new Buffer(0)
  process.stdout.write(empty, function() { 
    process.stderr.write(empty, function() { 
       process.exit(code);
    });
  });

doesn't reliable ensure that a consumer of node.js will get all data (yes - it is a PIPE, not a FILE !!).

@minesworld
Copy link
Author

Oh - and to be a bit bitchy - either this or #3669 and related must be open.

I would suggest to open the other ones again as just fixing the docs would mean that node.js is unusable for implementing cli tools.

@mscdex mscdex added the process Issues and PRs related to the process subsystem. label Nov 5, 2015
@ChALkeR ChALkeR added the doc Issues and PRs related to the documentations. label Nov 5, 2015
@Fishrock123
Copy link
Contributor

@minesworld please don't open duplicates, we can re-open issues if necessary. :)

The "fix" is #3170, i.e. fix the docs.

would mean that node.js is unusable for implementing cli tools.

process.exit() causes Node to terminate immediately, ignoring that data potentially hasn't been written places. The docs for process.exit() could probably be more clear.

You don't have to use .exit() to set an exit code -- you can just set process.exitCode. If you then return such that nothing else is being done, node will exit more gracefully.

The exact reason that it doesn't get written has something to do with chunked writes far lower down, probably near the OS layer. I'm not sure if this is actually properly fixable in libuv, see #1771 (comment) and libuv/libuv#428 -- ideally it's up to your application. In this case it means not immediately terminating the process on write.

@minesworld I haven't seen this happen for not-chunked writes before, perhaps that has to do with exiting in the cllback. Could you try wrapping the process.exit() in process.nextTick(), and if that doesn't work, setImmediate(), and seeing if that works?

Another option would be a process.exitGracefully(), although you may be able to write that in an npm module.

@bnoordhuis
Copy link
Member

Can I suggest we close this bug report? It's a known issue with several open bug reports. This one doesn't add anything new.

@Fishrock123
Copy link
Contributor

@bnoordhuis maybe we should add this to the known issues part of the release notes given how many times it has come up... cc @nodejs/release

@bnoordhuis
Copy link
Member

Good idea.

@minesworld
Copy link
Author

Do node.js a favour by:

  • write a note in the docs stating that the above code won't guarantee that all data is flushed
  • refer to that in Stream.write at the callback section
  • refer to that in process.stdout, process.stderr

Putting it into known issues would be a just "OK" solution. But writing it into the docs gives it the prominent place it deserves. Because this behaviour is totally against the docs.

It doesn't matter that you shouldn't call process.exit(), the code before should have ensured that all data to stdio is flushed. Also sometimes it isn't possible to not call process.exit().

I've looked at the node and libuv code but didn't get my head around it yet. Here are some suggestions which might help:

  • add an option to process.exit like process.exit(code, { flushStdIO:true}) and add some code which waits for that to happen. This would be 100% compatible with existing code which mostly aren't cli tools...
  • if there are doubts/issues about writing/reading from process stdio to/from files, it might be a solution to wrap node into some "/bin/cat -" calls. These don't cost any processing time at all but node.js would always have pipes to write to... . As node.js does call other (hardcoded) unix tools most people won't mind. And if it is an option to start node.js with it would make those who need it very happy...

@minesworld
Copy link
Author

I just want to express my thanks for NOT closing this issue and thinking about re-opening other issues again.

This here shouldn't have happened at all as I was the n person who experienced it and "wasted" a lot of time.

But better late then never.

For all of us out there using node.js a bit "different" - in my case its a ssh2 client - having these informations is really really crucial as it might affect the design phase etc.

Thats why I urge to put it into the docs...

@minesworld
Copy link
Author

If there is nobody to want to change the docs, let me know - I will fork it and do it for you.

At the moment I'm a bit under stress getting my ssh2 client working reliable as my current workaround to wait a bit for the data to be flushed before process.exit() needs some more work (as expected...)

@Fishrock123
Copy link
Contributor

Also sometimes it isn't possible to not call process.exit()

What's an example? I can't think of one unless your program is written such that you'd need to exit immediately to prevent other async calls for some reason.

If there is nobody to want to change the docs, let me know - I will fork it and do it for you.

@minesworld most of us a pretty busy and definitely wouldn't mind that. :)

@minesworld
Copy link
Author

@Fishrock123 when you haven't control over what other libraries are keeping in the background running...

I will fork node.js and update the docs ...

At the moment I'm using a workaround by listening on a pipe before process.stdout/.stderr to end and then wait 10 seconds before calling process.exit(). Its ugly and only 99,99% reliable and a waste of time if nothing big was written firsthand... but at least its working better than not using it.

@minesworld
Copy link
Author

Created a pull request: #3772

Hope that letting Node.js ending itself really ensures that all data to process.stdout/.stderr is flushed...

@Trott
Copy link
Member

Trott commented Jun 6, 2016

I think this can be closed because all the relevant activity seems to be at #6456 and this issue has been dormant for nearly 7 months. Feel free to re-open (or comment asking that it be re-opened) if you disagree.

@Trott Trott closed this as completed Jun 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

No branches or pull requests

6 participants