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

lib: fix process.send() sync i/o regression #774

Closed
wants to merge 1 commit into from

Conversation

bnoordhuis
Copy link
Member

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
Copy link
Member Author

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

@bnoordhuis
Copy link
Member Author

@brendanashworth
Copy link
Contributor

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
Copy link
Member

saghul commented Feb 10, 2015

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

@brendanashworth
Copy link
Contributor

@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.

var proc = fork(__filename, ['child']);

proc.on('message', common.mustCall(function(msg) {
assert.equal(typeof(msg), 'string');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

trevnorris pushed a commit to nodejs/node-v0.x-archive that referenced this pull request Feb 10, 2015
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
Copy link
Member Author

@vkurchatkin
Copy link
Contributor

LGTM

@bnoordhuis
Copy link
Member Author

@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
Copy link
Contributor

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

@rvagg rvagg mentioned this pull request Feb 11, 2015
trevnorris pushed a commit to nodejs/node-v0.x-archive that referenced this pull request Feb 11, 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.

PR-URL: nodejs/node#774
Reviewed-by: Trevor Norris <[email protected]>
@piscisaureus
Copy link
Contributor

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
Copy link
Contributor

@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).

trevnorris pushed a commit to nodejs/node-v0.x-archive that referenced this pull request Feb 12, 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.

PR-URL: nodejs/node#774
Reviewed-by: Trevor Norris <[email protected]>
@brendanashworth
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

@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.

misterdjules pushed a commit to misterdjules/node that referenced this pull request Feb 17, 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.

PR-URL: nodejs/node#774
Reviewed-by: Trevor Norris <[email protected]>
@rvagg
Copy link
Member

rvagg commented Feb 20, 2015

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).

@mscdex mscdex added the child_process Issues and PRs related to the child_process subsystem. label Mar 22, 2015
This was referenced Mar 23, 2015
@defunctzombie
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Member

rvagg commented Apr 28, 2015

@piscisaureus
Copy link
Contributor

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

@piscisaureus piscisaureus modified the milestones: 3.0.0, 2.0.0 Apr 30, 2015
@Fishrock123
Copy link
Contributor

@bnoordhuis will you have time to update this soon?

@bnoordhuis
Copy link
Member Author

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

@lewispham
Copy link

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
Copy link

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
Copy link
Member Author

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 Fishrock123 removed this from the 3.0.0 milestone May 26, 2015
@Fishrock123
Copy link
Contributor

Closing, possibly to be rediscussed in #34

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.