Skip to content
This repository has been archived by the owner on Oct 7, 2020. It is now read-only.

[Converge] lib: fix stdio/ipc sync i/o regression #34

Closed
jasnell opened this issue May 22, 2015 · 7 comments
Closed

[Converge] lib: fix stdio/ipc sync i/o regression #34

jasnell opened this issue May 22, 2015 · 7 comments

Comments

@jasnell
Copy link
Member

jasnell commented May 22, 2015

See nodejs/node-v0.x-archive@2411bea
/cc: @bnoordhuis @trevnorris

Original commit message:

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

See nodejs/node#774 (comment)

@bnoordhuis
Copy link
Member

Hah, that actually landed in joyent/node? Like I said in the other issue, I think it's better to embrace the asynchronous nature of process.send(). Now at least its behavior is consistent on Windows and UNIX.

@jasnell
Copy link
Member Author

jasnell commented May 27, 2015

Ok, so what do we want to do with this one then? It looks to me like landing this in node was likely premature given the additional discussion on the io.js side. This would mean a change of behavior from v0.12 to the first converged release so we'll need to document it.

@Fishrock123
Copy link
Contributor

I suggest we use the major for this as a breaking change and document it.

@jasnell
Copy link
Member Author

jasnell commented Jul 9, 2015

@Fishrock123 ... Ok, then I think this is added to the list of breaking changes from v0.12 to the converged release. /cc @misterdjules

I'm not sure if I quite understand the change enough myself to document it, however.

@rvagg
Copy link
Member

rvagg commented Aug 24, 2015

Moving discussion back to nodejs/node#760

tbh I don't really know where this stands, if we make it officially async do we have to change the non-windows implementations? we'd need to change the docs too I suppose. It would be great to get rid of the Known Issues item for this.

@brendanashworth
Copy link
Contributor

This became async in nodejs/node@56d9584, closing.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants