deps: uv 1.4.0 update #9179

trevnorris · 2015-02-10T18:25:28Z

Update libuv to 1.4.0 and land a fix that changed default behavior to preserve backwards compatibility.

bnoordhuis · 2015-02-10T19:16:43Z

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

+var proc = fork(__filename, ['child']);
+
+proc.on('message', common.mustCall(function(msg) {
+  assert.equal(typeof msg , 'string');


Style nit: space before comma. :-)

heh. thanks. I just did a replace on the () that were there.

misterdjules · 2015-02-10T19:55:04Z

I’m not sure about the impact of * win, unix: add pipe_peername implementation on backward compatibility. It's a new API that is not present in v0.12.0. Users could write native modules using this libuv API for node v0.12.1, but these modules would break on platforms where an older libuv 1.x is shipped as a dynamic library, and where Node.js is built to load libuv dynamically.

Are there platforms doing that currently, and if so what version of libuv do they track/ship?

EDIT: Regarding backward compatibility, I also wonder if libuv/libuv#156 could , by introducing an important behavior change that is sort of an API change, break users of such platforms.

misterdjules · 2015-02-10T19:57:30Z

Just for the record, the build fails on SmartOS currently.

This build issue had been fixed by the fact that the tty related change had been reverted. At some point later, it was put back, but in a version of libuv > 1.02 if I remember correctly. I had sent a PR that fixed the original breaking change, I'm not sure if it still applies though.

misterdjules · 2015-02-10T20:00:46Z

I think we should also track an issue to add dtrace probes support back. In my opinion it's very useful for some users and I would like to at least track it somewhere. Thoughts?

misterdjules · 2015-02-17T01:55:35Z

@trevnorris The SmartOS build is fixed by misterdjules@8ba154b, and I reworded the header of e0b44d6 into misterdjules@137d331.

The SmartOS build fix will need to be upstreamed, should we float the patch first and upstream it, or upstream it first so that we don't have to patch it (but that would delay the upgrade)?

tjfontaine · 2015-02-17T18:22:50Z

General Dependency Upgrades

Since Node.js doesn't currently define its own API/ABI the only way for users to build and ship binary addons is by building against the libuv and V8 that Node.js releases are built against (and in turn shipped). This also applies to c-ares, zlib, openssl, etc

We can upgrade any of our tightly coupled dependencies in a patch release, so long as those upgrades meet a couple requirements:

No public symbols that Node.js is linking against may be removed
All public symbols must behave in the way they did in the previous version, less any fixed bugs
- bugs are generally described as fixed behavior that wasn't working as documented, or that the user couldn't have been explicitly/implicitly depending on
No breaking ABI changes for artifacts currently built and linked against the old version

Presuming the upgrade to the library meets those requirements, the only follow up to that is:

Node.js must not use any new symbols/APIs

Since we support/allow building and linking against shared versions of our dependencies, consumers may not upgrade them in sync, so the node binary (and also their binary addons) must continue to work with whatever version of the shared library they have installed.

This Upgrade

From an API/ABI perspective most of this looks fine, save for the change to uv_tty_set_mode. I think in practice we're probably fine changing the int to enum but this is indeed one of those portions where the size of the enum is implementation defined, and can be one of those places where ABI is easily broken.

While there is a C++ level API compatibility change, what's not entirely clear is if the int to enum change is ABI compliant for people using only C.

I have not evaluated all the behavioral changes to know any further potential impacts.

misterdjules · 2015-02-17T22:15:00Z

Ran the node.js' tests with this upgrade and the fix for the SmartOS build, and so far on UNICEs all tests pass except for one test on SmartOS.

misterdjules · 2015-02-18T01:32:58Z

It also seems to break a lot of tests on Windows. I haven't had the time to investigate those though.

trevnorris · 2015-02-19T17:52:33Z

I might have missed a floating patch, though I thought that's what "lib: fix stdio/ipc sync i/o regression" was supposed to fix.

orangemocha · 2015-02-20T14:22:33Z

Investigating those Windows failures....

orangemocha · 2015-02-20T18:34:12Z

I haven't completed the investigation yet (I will resume on Monday), but I have determined that the second commit in this PR (42371f7) and not the libuv uddate is causing the WIndows child_process-related failures.
cc @bnoordhuis

bnoordhuis · 2015-02-21T00:19:23Z

See nodejs/node#774 - apparently blocking pipe I/O is not (always?) supported on Windows but to what extent is unclear to me.

orangemocha · 2015-03-03T16:43:16Z

I have opened a PR in libuv that addresses the blocking pipe issues on Windows. I think we'll have to float a patch on here. @trevnorris is it ok for me to add a commit to this PR?

trevnorris · 2015-03-03T19:40:07Z

@orangemocha be my guest.

trevnorris · 2015-03-03T19:49:24Z

@orangemocha Thanks for the PR, and great work! I've already cherry-picked your commit onto this PR.

@misterdjules This should be gtg for another test on Jenkins.

orangemocha · 2015-03-03T19:56:56Z

Thanks! Let's see what Jenkins says: http://jenkins.nodejs.org/job/node-accept-pull-request/26/ (testing only)

trevnorris · 2015-03-18T17:39:21Z

I could use some help on this one. I don't have OSX, and can't find a single change regards to a -static option.

Because we are floating several patches on top of libuv, make that apparent in the version number.

trevnorris · 2015-03-18T17:42:31Z

@saghul something like c515187

saghul · 2015-03-18T17:55:20Z

I could use some help on this one. I don't have OSX, and can't find a single change regards to a -static option.

It doesn't seem to be a libuv issue, but an issue on the OSX machine. All static libraries are failing to compile there.

saghul · 2015-03-18T17:55:44Z

@trevnorris yeah, that will do, thanks a lot!

misterdjules · 2015-03-18T18:05:51Z

@orangemocha @trevnorris @saghul The build issue on OSX was due to a problem in the build slave configuration, and I'm the one who broke it when fixing the libuv jenkins job. My apologies for that. It should be fixed now, but let me know if you still have issues with it. Sorry again!

misterdjules · 2015-03-18T18:07:12Z

@saghul @trevnorris FWIW, I'm fine with @saghul's suggestion in #9179 (comment) and with @trevnorris' change.

misterdjules · 2015-03-18T18:53:22Z

@orangemocha @trevnorris @saghul All tests pass on supported unices.

misterdjules · 2015-03-18T19:12:11Z

deps/uv/include/uv.h

@@ -138,6 +138,7 @@ extern "C" {
  XX(EOF, "end of file")                                                      \
  XX(ENXIO, "no such device or address")                                      \
  XX(EMLINK, "too many links")                                                \
+  XX(EHOSTDOWN, "host is down")                                               \


That seems like another API change that might be a bit more difficult not to use within the node code base, but at that point I don't know what other options we have.

misterdjules · 2015-03-18T19:29:30Z

@joyent/node-coreteam besides the API changes, including the latest one in a 1.4.x patch release, LGTM.

Do we have any option other than just landing these API changes in node? Any other way of moving forward I can think of requires a lot of maintenance from node for which we don't have proper tooling right now.

saghul · 2015-03-19T10:39:30Z

@misterdjules: That change is somewhere between a bugfix and an API change. libuv doesn't use the symbol but some OS-es generate it so we now export it. The alternative is an assert, which quite some people where getting while downloading packages with NPM, for example, which is not desired.

FWIW this particular change can be "ifdef tested".

misterdjules · 2015-03-19T17:59:31Z

@trevnorris Assigned to you, let me know if you have any trouble running the node-accept-pull-request job on Jenkins. Thank you!

misterdjules · 2015-03-19T18:10:42Z

@saghul OK, my concern was more that someone using UV_EHOSTDOWN with a given v1.4.x release will get a compilation error with another v1.4.x release that doesn't define this value, unless I'm missing something.

saghul · 2015-03-19T18:58:08Z

@misterdjules that is indeed a possibility, unfortunately.

Reviewed-By: Julien Gilli <[email protected]> PR-URL: #9179

The -fno-strict-aliasing flag was added to fix compilation warnings when building Node.js with GCC <= 4.4 Reviewed-By: Julien Gilli <[email protected]> PR-URL: #9179

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. Reviewed-By: Julien Gilli <[email protected]> PR-URL: #9179

Float patch to fix pipe on Windows. Original commit message: win: fix pipe blocking writes In the code path for pipe blocking writes, WriteFile is already posting a completion packet to the I/O completion port. POST_COMPLETION_FOR_REQ was causing the same request to get returned twice by GetCompletionStatusEx. Also on the same code path, we were waiting on the wrong event. We need to update queued_bytes and write_queue_size when a blocking write request completes asynchronously. Ref: libuv/libuv#238 Reviewed-By: Julien Gilli <[email protected]> PR-URL: #9179

Float patch to fix setsockopt for multicast on Solaris and derivatives. Original commit message: solaris: fix setsockopt for multicast options On Solaris and derivatives such as SmartOS, the length of socket options for multicast and ttl options is not always sizeof(char). This fixes the udp_options and udp_options6 tests. Ref: libuv/libuv#243 Reviewed-By: Julien Gilli <[email protected]> PR-URL: #9179

Original commit message: darwin: fix size calculation in select() fallback Apple's `fd_set` stores its bits in an array of 32-bit integers, which means `FD_ISSET()` may read out of bounds if we allocate storage at byte granularity. There's also a chance that the `select()` call could corrupt the heap, although I didn't investigate that. This issue was discovered by LLVM's AddressSanitizer which caught `FD_ISSET()` trying to read out of bounds. Ref: libuv/libuv#241 Reviewed-By: Julien Gilli <[email protected]> PR-URL: #9179

Because we are floating several patches on top of libuv, make that apparent in the version number. Reviewed-By: Julien Gilli <[email protected]> PR-URL: #9179

misterdjules · 2015-03-27T23:00:09Z

Landed in 7a37910, c0766eb, 2411bea, 3cce8ab, 9613ac7, 2fdeb7e and a930870.

Thank you @trevnorris @saghul, @orangemocha and everyone involved 👍

The -fno-strict-aliasing flag was added to fix compilation warnings when building Node.js with GCC <= 4.4 Reviewed-By: Julien Gilli <[email protected]> PR-URL: nodejs#9179

The -fno-strict-aliasing flag was added to fix compilation warnings when building Node.js with GCC <= 4.4 Reviewed-By: Julien Gilli <[email protected]> PR-URL: nodejs#9179 PR: nodejs#25141 PR-URL: nodejs#25141 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Julien Gilli <[email protected]>

Nodejs-Jenkins added pr v0.12 labels Feb 10, 2015

orangemocha added the in progress label Feb 10, 2015

misterdjules added this to the 0.12.1 milestone Feb 10, 2015

trevnorris mentioned this pull request Feb 10, 2015

deps: updated libuv to 1.3.0 #9165

Closed

bnoordhuis reviewed Feb 10, 2015
View reviewed changes

trevnorris force-pushed the uv-1.4.0-update branch 2 times, most recently from 7ffc17a to 6e21bd0 Compare February 11, 2015 17:33

trevnorris mentioned this pull request Feb 11, 2015

process.stdout.on('resize') delayed ~100ms #9032

Closed

trevnorris force-pushed the uv-1.4.0-update branch from 6e21bd0 to 42371f7 Compare February 12, 2015 00:10

orangemocha self-assigned this Mar 2, 2015

orangemocha mentioned this pull request Mar 3, 2015

win: fix pipe blocking writes libuv/libuv#238

Closed

trevnorris force-pushed the uv-1.4.0-update branch 2 times, most recently from 9b4db67 to 14f00ed Compare March 3, 2015 19:48

deps: indicate uv has been patched

c515187

Because we are floating several patches on top of libuv, make that apparent in the version number.

misterdjules reviewed Mar 18, 2015
View reviewed changes

misterdjules assigned trevnorris Mar 19, 2015

trevnorris added a commit that referenced this pull request Mar 19, 2015

deps: update libuv to 1.4.2

7a37910

Reviewed-By: Julien Gilli <[email protected]> PR-URL: #9179

trevnorris added a commit that referenced this pull request Mar 19, 2015

build: fix use of strict aliasing

c0766eb

The -fno-strict-aliasing flag was added to fix compilation warnings when building Node.js with GCC <= 4.4 Reviewed-By: Julien Gilli <[email protected]> PR-URL: #9179

trevnorris added a commit that referenced this pull request Mar 19, 2015

deps: indicate uv has been patched

a930870

Because we are floating several patches on top of libuv, make that apparent in the version number. Reviewed-By: Julien Gilli <[email protected]> PR-URL: #9179

misterdjules closed this Mar 27, 2015

misterdjules removed the in progress label Mar 27, 2015

saghul mentioned this pull request Mar 29, 2015

uv-common.c:75: uv_err_name: Assertion `0' failed. libuv/libuv#251

Closed

This was referenced Mar 30, 2015

Binding http server to file descriptor fails to read #14183

Closed

node-fastcgi compatibility with node ≥ 0.11.8 #8988

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deps: uv 1.4.0 update #9179

deps: uv 1.4.0 update #9179

trevnorris commented Feb 10, 2015

bnoordhuis Feb 10, 2015

trevnorris Feb 10, 2015

misterdjules commented Feb 10, 2015

misterdjules commented Feb 10, 2015

misterdjules commented Feb 10, 2015

misterdjules commented Feb 17, 2015

tjfontaine commented Feb 17, 2015

misterdjules commented Feb 17, 2015

misterdjules commented Feb 18, 2015

trevnorris commented Feb 19, 2015

orangemocha commented Feb 20, 2015

orangemocha commented Feb 20, 2015

bnoordhuis commented Feb 21, 2015

orangemocha commented Mar 3, 2015

trevnorris commented Mar 3, 2015

trevnorris commented Mar 3, 2015

orangemocha commented Mar 3, 2015

trevnorris commented Mar 18, 2015

trevnorris commented Mar 18, 2015

saghul commented Mar 18, 2015

saghul commented Mar 18, 2015

misterdjules commented Mar 18, 2015

misterdjules commented Mar 18, 2015

misterdjules commented Mar 18, 2015

misterdjules Mar 18, 2015

trevnorris Mar 18, 2015

misterdjules commented Mar 18, 2015

saghul commented Mar 19, 2015

misterdjules commented Mar 19, 2015

misterdjules commented Mar 19, 2015

saghul commented Mar 19, 2015

misterdjules commented Mar 27, 2015

deps: uv 1.4.0 update #9179

deps: uv 1.4.0 update #9179

Conversation

trevnorris commented Feb 10, 2015

bnoordhuis Feb 10, 2015

Choose a reason for hiding this comment

trevnorris Feb 10, 2015

Choose a reason for hiding this comment

misterdjules commented Feb 10, 2015

misterdjules commented Feb 10, 2015

misterdjules commented Feb 10, 2015

misterdjules commented Feb 17, 2015

tjfontaine commented Feb 17, 2015

General Dependency Upgrades

This Upgrade

misterdjules commented Feb 17, 2015

misterdjules commented Feb 18, 2015

trevnorris commented Feb 19, 2015

orangemocha commented Feb 20, 2015

orangemocha commented Feb 20, 2015

bnoordhuis commented Feb 21, 2015

orangemocha commented Mar 3, 2015

trevnorris commented Mar 3, 2015

trevnorris commented Mar 3, 2015

orangemocha commented Mar 3, 2015

trevnorris commented Mar 18, 2015

trevnorris commented Mar 18, 2015

saghul commented Mar 18, 2015

saghul commented Mar 18, 2015

misterdjules commented Mar 18, 2015

misterdjules commented Mar 18, 2015

misterdjules commented Mar 18, 2015

misterdjules Mar 18, 2015

Choose a reason for hiding this comment

trevnorris Mar 18, 2015

Choose a reason for hiding this comment

misterdjules commented Mar 18, 2015

saghul commented Mar 19, 2015

misterdjules commented Mar 19, 2015

misterdjules commented Mar 19, 2015

saghul commented Mar 19, 2015

misterdjules commented Mar 27, 2015