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

Chunked stdout/stderr drops writes if terminated early. #784

Closed
jshkurti opened this issue Feb 10, 2015 · 28 comments
Closed

Chunked stdout/stderr drops writes if terminated early. #784

jshkurti opened this issue Feb 10, 2015 · 28 comments
Labels
confirmed-bug Issues with confirmed bugs. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. process Issues and PRs related to the process subsystem.

Comments

@jshkurti
Copy link

Hello,

I have an app which prints a long json to the output and I need to | grep this output in order to parse some data.
It works fine with Node.js but it doesn't with iojs.
It seems the output is chunked in some ways and grep stops before receiving all the data.
I came to this conclusion because when I redirect the output in some file and then cat file | grep it works, everything is there, but iojs app.js | grep won't.

Any ideas on this issue ?
Thanks.

@Fishrock123
Copy link
Contributor

@jshkurti Did you try with node 0.12?

@jshkurti
Copy link
Author

With Node 0.10.x 0.11.x and 0.12.x yeah :)

@jshkurti
Copy link
Author

Actually, the background story here is that I'm working on https://github.com/Unitech/PM2 and tests don't pass with iojs.
PM2 seems to work fine with iojs though so I investigated a bit and found out that tests don't pass because of the grep issue.

@vkurchatkin
Copy link
Contributor

Could it be related to #760?

@vkurchatkin
Copy link
Contributor

A test (iojs test.js | grep hey):

var str = '';

for (var i = 0; i < 1000000; i++) {
  str += 'test\n'
}

str += 'hey\n';

process.stdout.write(str);
process.exit();

@vkurchatkin
Copy link
Contributor

Looks like a fix:

diff --git a/lib/net.js b/lib/net.js
index 030083d..efebd03 100644
--- a/lib/net.js
+++ b/lib/net.js
@@ -135,8 +135,7 @@ function Socket(options) {
     this._handle = createHandle(options.fd);
     this._handle.open(options.fd);
     if ((options.fd == 1 || options.fd == 2) &&
-        (this._handle instanceof Pipe) &&
-        process.platform === 'win32') {
+        (this._handle instanceof Pipe)) {
       // Make stdout and stderr blocking on Windows
       var err = this._handle.setBlocking(true);
       if (err)

/cc @bnoordhuis

@jshkurti
Copy link
Author

Tested with iojs-1.0.4, doesn't work either.

@vkurchatkin your solution works, thanks :)
It should be on the next release then I guess ?

@vkurchatkin vkurchatkin reopened this Feb 10, 2015
@vkurchatkin
Copy link
Contributor

It's not in yet, and I'm not sure it is a good solution, just a guess. let's wait for @bnoordhuis

@bnoordhuis
Copy link
Member

It looks correct to me. I've looked at that code when I put together #774 but I wasn't completely sure if it needed updating. This bug report is good external validation. I'll update the PR with the proposed change and a regression test. :-)

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Feb 10, 2015
process.send() should be synchronous, it should block until the message
has been sent in full, but it wasn't after the second-to-last libuv
upgrade because of commit libuv/libuv@393c1c5 ("unix: set non-block
mode in uv_{pipe,tcp,udp}_open"), which made its way into io.js in
commit 07bd05b ("deps: update libuv to 1.2.1").

Commit libuv/libuv@b36d4ff ("unix: implement uv_stream_set_blocking()")
as landed in io.js in commit 9681fca ("deps: update libuv to 1.4.0")
makes it possible to restore the synchronous behavior again and that's
precisely what this commit does.

The same line of reasoning applies to `net.Socket({ fd: 1 })`: creating
a socket object from a stdio file descriptor, like the `process.stdout`
getter does, should put the file descriptor in blocking mode for
compatibility reasons.

Fixes: nodejs#760
Fixes: nodejs#784
@Fishrock123 Fishrock123 added the confirmed-bug Issues with confirmed bugs. label May 6, 2015
Fishrock123 added a commit to Fishrock123/node that referenced this issue May 22, 2015
Fishrock123 added a commit to Fishrock123/node that referenced this issue May 25, 2015
Fishrock123 added a commit to Fishrock123/node that referenced this issue May 25, 2015
Fishrock123 added a commit to Fishrock123/node that referenced this issue May 25, 2015
@brendanashworth
Copy link
Contributor

This is a known issue of io.js, but this is technically a duplicate of #760 so I'm going to close this in favor of tracking it in the other issue. Thank you for reporting this!

@brendanashworth brendanashworth added the duplicate Issues and PRs that are duplicates of other issues or PRs. label Jun 24, 2015
@Fishrock123
Copy link
Contributor

@brendanashworth I'm really not sure it is actually a duplicate, see: #1771

Reopening for now.

@Fishrock123 Fishrock123 reopened this Jun 25, 2015
@brendanashworth brendanashworth removed the duplicate Issues and PRs that are duplicates of other issues or PRs. label Jun 25, 2015
@brendanashworth
Copy link
Contributor

Totally sorry! You're right, I should have looked more closely.

@Fishrock123
Copy link
Contributor

So in #1771 I describe that this only happens if your process exists prematurely. (I.e. process.exit() before all chunked writes are finished. @jshkurti Are you sure the current behavior isn't expected? Any idea what PM2 would be doing to cause that?

@Fishrock123
Copy link
Contributor

I never paid attention to this but WriteableStream.write is definitely non-blocking.

That function, and the streams state may be, but the crux of this issue is lower.

I guess the process.exit operation takes a bit of time and reveals the problem if the write+flush takes more time than the process to exit.

I would say the opposite is true. exit() is virtually instantaneous. And if flushing hasn't been done at an OS level, it will still drop chunked writes.

Did you check if your new examples worked? It's probable those only fire the callback after the stream has flushed, but not after chunked writes have actually been written/flushed to stdio.

@evanlucas
Copy link
Contributor

@Fishrock123 Can this be closed now that stdio is blocking?

@TimothyGu
Copy link
Member

@evanlucas, only stdio that are redirected to TTYs is blocking. @vkurchatkin's test case uses pipes, which still don't work.

@Trott Trott added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Jul 30, 2017
ryzokuken added a commit to ryzokuken/node that referenced this issue Mar 6, 2018
Rename the test appropriately alongside mentioning the subsystem
Also, make a few basic changes to make sure the test conforms
to the standard test structure

Refs: nodejs#19105
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure
devsnek pushed a commit that referenced this issue Mar 8, 2018
Rename the test appropriately alongside mentioning the subsystem
Also, make a few basic changes to make sure the test conforms
to the standard test structure

Refs: #19105
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure

PR-URL: #19161
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
targos pushed a commit that referenced this issue Mar 17, 2018
Rename the test appropriately alongside mentioning the subsystem
Also, make a few basic changes to make sure the test conforms
to the standard test structure

Refs: #19105
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure

PR-URL: #19161
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 20, 2018
Rename the test appropriately alongside mentioning the subsystem
Also, make a few basic changes to make sure the test conforms
to the standard test structure

Refs: #19105
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure

PR-URL: #19161
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@ryzokuken
Copy link
Contributor

Is this still not fixed?

@addaleax
Copy link
Member

addaleax commented Apr 3, 2018

@ryzokuken No, and it’s hard to fix this. I have some thoughts on how to tackle this after we can get #19377 in, though:

I think we could flush the stdio streams as part of the handle cleanup methods (or possibly even all libuv-backed streams), by delaying closing those streams until after all libuv requests have drained from the event loop. We’d need a way to read all the pending stream.Writable JS-land buffers from C++, but I think that should be doable.

If we can get that going, we might even want to look into doing more async stdio again by default (because that’s Node.js’s general principle).

@ryzokuken
Copy link
Contributor

ryzokuken commented Apr 3, 2018

hmm, this does seem complicated. I have been looking into the issue tracker trying to clean up as much as possible, so I'd be commenting a lot on issues I have next to no idea about anyway 😅

That said, I'd love to take this up on another day if it's still not fixed till then, once I have a deeper understanding of the inner-workings of libuv.

@addaleax
Copy link
Member

addaleax commented Apr 3, 2018

@ryzokuken Yea, that sounds like a good idea – and if you want to actually bring some of these issues to a conclusion, that would be amazing. 💙

@ryzokuken
Copy link
Contributor

Yes, that's the plan.

MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
Rename the test appropriately alongside mentioning the subsystem
Also, make a few basic changes to make sure the test conforms
to the standard test structure

Refs: nodejs#19105
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure

PR-URL: nodejs#19161
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
trivikr pushed a commit to trivikr/node that referenced this issue Sep 15, 2018
Rename the test appropriately alongside mentioning the subsystem
Also, make a few basic changes to make sure the test conforms
to the standard test structure

Refs: nodejs#19105
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure

PR-URL: nodejs#19161
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@jasnell
Copy link
Member

jasnell commented Jun 19, 2020

There's been no further activity on this in over 2 years. Closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.