lib: fix process.send() sync i/o regression by bnoordhuis · Pull Request #774 · nodejs/node

bnoordhuis · 2015-02-09T21:53:33Z

process.send() should be synchronous, i.e. 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.

Fixes: #760

R=@brendanashworth @piscisaureus

On Windows, this sets the UV_HANDLE_BLOCKING_WRITES flag on the handle. I'd like @piscisaureus to confirm whether that's harmless or not.

https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/160/

bnoordhuis · 2015-02-09T21:55:19Z

The CI run seems to be conflicting with an already running one. I'll resubmit it in a bit.

bnoordhuis · 2015-02-09T22:34:52Z

https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/162/

brendanashworth · 2015-02-10T02:12:01Z

Running those changes on my 10.9.5 mac laptop:

Apple LLVM version 6.0 (clang-600.0.45.3) (based on LLVM 3.5svn)
Target: x86_64-apple-darwin13.4.0

I get tons of failed tests for some reason, all with either TIMEDOUT errors or with ENOSYS such as the following. While trying the use case as given in #760, I get the following error (much alike the test failing errors):

child_process.js:574
    throw errnoException(err, 'setBlocking');
          ^
Error: setBlocking ENOSYS
    at exports._errnoException (util.js:734:11)
    at Object.exports._forkChild (child_process.js:574:11)
    at Function.startup.processChannel (node.js:689:10)
    at startup (node.js:41:15)
    at node.js:799:3

saghul · 2015-02-10T07:54:39Z

@brendanashworth are you sure you have the uv upgrade patch applied?

brendanashworth · 2015-02-10T08:03:15Z

@saghul oop, you're right, I needed to update my master before I branched to test this - my fault. On my system this now fixes it. <3

Besides the typeof nitpick the code looks good to me.

brendanashworth · 2015-02-10T08:04:57Z

test/parallel/test-child-process-sync-process-send.js

is there a reason typeof(msg) is used here instead of typeof msg? I usually see the latter in the codebase

No reason. In my head it's more of a function call than an operator. I'll drop the parens before I land it.

process.send() should be synchronous, i.e. 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. PR-URL: nodejs/node#774 Reviewed-by: Trevor Norris <[email protected]>

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

bnoordhuis · 2015-02-10T21:04:50Z

Updated with the feedback from #784. PTAL @brendanashworth @vkurchatkin.

https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/163/

vkurchatkin · 2015-02-10T21:14:50Z

LGTM

bnoordhuis · 2015-02-10T21:55:16Z

@piscisaureus mentioned in passing something about blocking mode being unsafe on Windows but without going into details. I've asked him to chime in.

brendanashworth · 2015-02-10T22:02:05Z

I haven't run the tests but the code looks great to me.

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. PR-URL: nodejs/node#774 Reviewed-by: Trevor Norris <[email protected]>

piscisaureus · 2015-02-11T18:18:38Z

On windows, process.send() has always been async. This bothers me and I wouldn't mind fixing it, but be aware that changing this now amounts to an API change.

Doing synchronous writes to a pipe that's open in "overlapped" mode is currently not supported by libuv, we'd have to add support for it.

sam-github · 2015-02-11T18:58:55Z

@piscisaureus this is one of those areas where its not clear. this could be called a major revision/api change, or it could be called a bug that windows is not same as linux, and only a patch... It could also be considered a regression, because maybe unix should be async on process.send (and stdout/err), and this goes the other way.

I don't have strong feelings. Its hard to know how many users depend on this sync feature on windows. I'd be tempted to call it a bug fix, for now, and make async on both in the next major (or streaming... but that's lots of work).

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. PR-URL: nodejs/node#774 Reviewed-by: Trevor Norris <[email protected]>

brendanashworth · 2015-02-13T00:17:13Z

In the cluster module docs, no mention of async / sync is mentioned. However in the child_process docs, it explicitly declares the method as synchronous:

Please note that the send() method on both the parent and child are synchronous - sending large chunks of data is not advised (pipes can be used instead, see child_process.spawn).

With that, I'd say that Windows being async would be a bug and should be a patch. Plus, since process.send doesn't take a callback anyways, this async -> sync change wouldn't break any code unless you depended on windows machines dropping data of large length - which kind of implies that you already know that it's a bug.

bnoordhuis · 2015-02-13T14:56:11Z

Let's discuss at the next TC meeting.

Doing synchronous writes to a pipe that's open in "overlapped" mode is currently not supported by libuv, we'd have to add support for it.

That might be problematic, it sounds like it could break the child_process module. The libuv documentation for uv_pipe_open() doesn't mention that limitation, by the way.

sam-github · 2015-02-13T21:51:09Z

@brendanashworth the cluster docs you link to say This function is equal to the send methods provided by child_process.fork()., it should be more clear, in particular, it should link to the child_process.send docs, to make clear that equal to means that it is literally the same method.

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. PR-URL: nodejs/node#774 Reviewed-by: Trevor Norris <[email protected]>

rvagg · 2015-02-20T01:47:02Z

status? this is going back into "known issues" for 1.3.0 where we mentioned that a fix should "appear in the next patch release" (my words I think, sorry).

defunctzombie · 2015-04-08T21:24:21Z

If the send is synchronous, does that mean it also waits in the receiver to process the message or just the sending side blocks to perform the send to the kernel?

bnoordhuis · 2015-04-08T22:08:33Z

The latter. There is no guarantee the receiver actually received it (or will receive it - the message is lost when the process goes away.)

rvagg · 2015-04-28T09:05:37Z

I did a simple merge of this and the latest master and ran a CI @ https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/632/ on my branch and got a bunch of failures on windows:

/cc @piscisaureus

piscisaureus · 2015-04-30T21:10:25Z

This is going to slip for the 2.0 release as decided on the TC meeting on 4/29. Removing it from the milestone.

Fishrock123 · 2015-05-19T16:54:25Z

@bnoordhuis will you have time to update this soon?

bnoordhuis · 2015-05-22T11:30:58Z

I think I'd rather take a different approach and make the asynchronous nature of process.send() official.

lewispham · 2015-05-26T08:54:48Z

There will be a huge pain to my application if process.send is being synchronous. I'm using this method for finding the appropriate WebSocket's clients in my multi-workers chat app an so on.

That being said, I suggest to let this method asynchronously by default .Synchronous message exchange should be applied via a flag or even another process.sendSync method.

YurySolovyov · 2015-05-26T09:05:25Z

Just curious: how well it will perform while being async?
I, personally don't need to send huge amounts of data, but someone else could...

Also, I think there are many userland modules, that are basically meant to do same thing as process.send() but in async way.

bnoordhuis · 2015-05-26T09:47:22Z

Performance-wise, it probably doesn't matter much if process.send() is synchronous or not; with most workloads, JSON.parse and JSON.stringify are the biggest cost centers.

Fishrock123 · 2015-07-01T18:41:53Z

Closing, possibly to be rediscussed in #34

brendanashworth reviewed Feb 10, 2015
View reviewed changes

bnoordhuis mentioned this pull request Feb 10, 2015

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

Closed

bnoordhuis force-pushed the fix-issue-760 branch from f366417 to 1fea458 Compare February 10, 2015 21:03

rvagg mentioned this pull request Feb 11, 2015

Release proposal: v1.2.0 #768

Closed

bnoordhuis added the tc-agenda label Feb 13, 2015

This was referenced Feb 13, 2015

doc: link cluster worker.send() to child.send() #839

Closed

doc: add releases document detail release cycle #630

Closed

rvagg mentioned this pull request Feb 18, 2015

io.js TC Meeting 2015-02-18 #874

Closed

rvagg mentioned this pull request Feb 20, 2015

Release proposal: v1.3.0 #872

Closed

bnoordhuis mentioned this pull request Feb 21, 2015

deps: uv 1.4.0 update nodejs/node-v0.x-archive#9179

Closed

mscdex added the child_process Issues and PRs related to the child_process subsystem. label Mar 22, 2015

This was referenced Mar 23, 2015

Release proposal: v1.6.2 #1235

Closed

Release proposal: v1.6.3 #1252

Closed

Fishrock123 mentioned this pull request Apr 5, 2015

Release proposal: v1.6.4 #1340

Closed

jihchi mentioned this pull request Apr 15, 2015

翻譯 io.js 3/20 週報 nodejs/nodejs-zh-TW#75

Merged

chrisdickinson mentioned this pull request Apr 17, 2015

Release proposal: 1.8.0 #1436

Closed

chrisdickinson added the land-on-master label Apr 17, 2015

yosuke-furukawa added the confirmed-bug label Apr 20, 2015

brendanashworth removed the confirmed-bug label Apr 21, 2015

rvagg mentioned this pull request Apr 27, 2015

Release proposal: v2.0.0 #1532

Closed

monsanto mentioned this pull request Apr 28, 2015

fs.writeSync truncates long lines #1541

Closed

piscisaureus modified the milestones: 3.0.0, 2.0.0 Apr 30, 2015

Fishrock123 mentioned this pull request May 15, 2015

Release proposal: v2.0.2 #1679

Merged

Fishrock123 mentioned this pull request May 22, 2015

[Converge] lib: fix stdio/ipc sync i/o regression nodejs/node-convergence-archive#34

Closed

Fishrock123 removed this from the 3.0.0 milestone May 26, 2015

jshkurti mentioned this pull request Jun 24, 2015

Asynchronous write() bug #2049

Closed

Fishrock123 closed this Jul 1, 2015

Fishrock123 mentioned this pull request Jul 1, 2015

process.send() is not synchronous #760

Closed

rvagg mentioned this pull request Jul 2, 2015

Release proposal: v2.3.2 #2083

Closed

Uh oh!

Conversation

bnoordhuis commented Feb 9, 2015

Uh oh!

bnoordhuis commented Feb 9, 2015

Uh oh!

bnoordhuis commented Feb 9, 2015

Uh oh!

brendanashworth commented Feb 10, 2015

Uh oh!

saghul commented Feb 10, 2015

Uh oh!

brendanashworth commented Feb 10, 2015

Uh oh!

brendanashworth Feb 10, 2015

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Feb 10, 2015

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Feb 10, 2015

Uh oh!

vkurchatkin commented Feb 10, 2015

Uh oh!

bnoordhuis commented Feb 10, 2015

Uh oh!

brendanashworth commented Feb 10, 2015

Uh oh!

piscisaureus commented Feb 11, 2015

Uh oh!

sam-github commented Feb 11, 2015

Uh oh!

brendanashworth commented Feb 13, 2015

Uh oh!

bnoordhuis commented Feb 13, 2015

Uh oh!

sam-github commented Feb 13, 2015

Uh oh!

rvagg commented Feb 20, 2015

Uh oh!

defunctzombie commented Apr 8, 2015

Uh oh!

bnoordhuis commented Apr 8, 2015

Uh oh!

rvagg commented Apr 28, 2015

Uh oh!

piscisaureus commented Apr 30, 2015

Uh oh!

Fishrock123 commented May 19, 2015

Uh oh!

bnoordhuis commented May 22, 2015

Uh oh!

lewispham commented May 26, 2015

Uh oh!

YurySolovyov commented May 26, 2015

Uh oh!

bnoordhuis commented May 26, 2015

Uh oh!

Fishrock123 commented Jul 1, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants