Libuv: Release proposal: v1.21.0

[cjihrig]

It's been about a month and a half since the last release. I'd like to do this later today or tomorrow unless there are specific objections.

CI: https://ci.nodejs.org/view/libuv/job/libuv-in-node/32/

[cjihrig]

It looks like we've introduced a regression in the Node.js test suite, pretty much across the board:

09:05:01 not ok 1228 parallel/test-net-socket-constructor
09:05:01   ---
09:05:01   duration_ms: 0.230
09:05:01   severity: fail
09:05:01   exitcode: 1
09:05:01   stack: |-
09:05:01     assert.js:80
09:05:01       throw new AssertionError(obj);
09:05:01       ^
09:05:01     
09:05:01     AssertionError [ERR_ASSERTION]: Input A expected to strictly equal input B:
09:05:01     + expected - actual
09:05:01     
09:05:01     - false
09:05:01     + true
09:05:01         at test (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd11-x64/test/parallel/test-net-socket-constructor.js:16:10)
09:05:01         at Server.net.createServer.common.mustCall (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd11-x64/test/parallel/test-net-socket-constructor.js:29:5)
09:05:01         at Server.<anonymous> (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd11-x64/test/common/index.js:468:15)
09:05:01         at Server.emit (events.js:182:13)
09:05:01         at TCP.onconnection (net.js:1561:8)

[santigimeno]

After this commit, the test is no longer correct as we're watching the same fd twice. I'll try to rework the test.

[cjihrig]

I opened https://github.com/nodejs/node/pull/21428, so that test should throw in several places due to uv_tcp_open() failing. Previously, it was failing silently, leading to the assertion error shown in my previous comment.

Should we move forward with this release as is and change the Node test, or is the change in 2256be01b0427ecfeb311c1af7e6bd079e0dc399 breaking enough to revert (or otherwise update) first? cc: @bnoordhuis who authored the commit in question.

[bnoordhuis]

It's helping unearth bugs. That's not breaking, that's progress. :-)

[santigimeno]

@cjihrig I think https://github.com/santigimeno/node/commit/dcee84c43990eb5ace5c15a4385c1d4c9c4a1f8f should fix the test. I have tested it locally on Linux with your changes from https://github.com/nodejs/node/pull/21428 and it seems to work.

[cjihrig]

Thanks @santigimeno. I ran your branch through the CI: https://ci.nodejs.org/view/libuv/job/libuv-in-node/33/. Looks like https://github.com/nodejs/node/pull/21428 and https://github.com/libuv/libuv/commit/2256be01b0427ecfeb311c1af7e6bd079e0dc399 don't work well together:

08:47:59 not ok 251 parallel/test-child-process-stdout-ipc
08:47:59   ---
08:47:59   duration_ms: 0.276
08:47:59   severity: fail
08:47:59   exitcode: 1
08:47:59   stack: |-
08:47:59     net.js:266
08:47:59         err = this._handle.open(fd);
08:47:59                            ^
08:47:59     
08:47:59     Error: EEXIST: file already exists, uv_pipe_open
08:47:59         at new Socket (net.js:266:24)
08:47:59         at createWritableStdioStream (internal/process/stdio.js:186:16)
08:47:59         at process.getStdout [as stdout] (internal/process/stdio.js:20:14)
08:47:59         at console.js:424:19
08:47:59         at NativeModule.compile (internal/bootstrap/loaders.js:235:7)
08:47:59         at Function.NativeModule.require (internal/bootstrap/loaders.js:155:18)
08:47:59         at setupGlobalConsole (internal/bootstrap/node.js:380:41)
08:47:59         at startup (internal/bootstrap/node.js:124:7)
08:47:59         at bootstrapNodeJSCore (internal/bootstrap/node.js:584:3)
08:47:59     assert.js:80
08:47:59       throw new AssertionError(obj);
08:47:59       ^
08:47:59     
08:47:59     AssertionError [ERR_ASSERTION]: Input A expected to strictly equal input B:
08:47:59     + expected - actual
08:47:59     
08:47:59     - 1
08:47:59     + 0
08:47:59         at ChildProcess.<anonymous> (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/test/parallel/test-child-process-stdout-ipc.js:17:10)
08:47:59         at ChildProcess.<anonymous> (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/test/common/index.js:468:15)
08:47:59         at ChildProcess.emit (events.js:182:13)
08:47:59         at Process.ChildProcess._handle.onexit (internal/child_process.js:237:12)

[santigimeno]

It seems we should be checking if process.env.NODE_CHANNEL_FD is set to one of the stdio descriptors when setting up those streams here.??
Disregard that comment, though the problem seems to be that fd: 1 is used first for ipc and later for setting up the process stdout.

[santigimeno]

This patch is working for me though I don't know how correct it is:

diff --git a/lib/internal/process/stdio.js b/lib/internal/process/stdio.js
index 6ee7566408..b8b20d127d 100644
--- a/lib/internal/process/stdio.js
+++ b/lib/internal/process/stdio.js
@@ -163,6 +163,12 @@ function setupProcessStdio({ getStdout, getStdin, getStderr }) {

 function createWritableStdioStream(fd) {
   var stream;
+  if (process.channel && process.channel.fd === fd) {
+    stream = process.channel;
+    stream.write = stream.writeUtf8String;
+    return stream;
+  }
+
   const tty_wrap = process.binding('tty_wrap');

   // Note stream._type is used for test-module-load-list.js

[cjihrig]

@santigimeno what do you think of https://github.com/cjihrig/node-1/commit/574cd7700235d9650edf4fba910fe793aef62564? It's a similar idea, but more like the existing check that's in place for stdin.

I think between https://github.com/cjihrig/node-1/commit/574cd7700235d9650edf4fba910fe793aef62564 and https://github.com/santigimeno/node/commit/dcee84c43990eb5ace5c15a4385c1d4c9c4a1f8f (I haven't tested it, just taking your word for it), we should be good to go ahead with the release.

[santigimeno]

@santigimeno what do you think of cjihrig/node-1@574cd77? It's a similar idea, but more like the existing check that's in place for stdin.

Yes, that's the correct fix I was looking for! 👍

I think between cjihrig/node-1@574cd77 and santigimeno/node@dcee84c (I haven't tested it, just taking your word for it), we should be good to go ahead with the release.

Yes, I think so but maybe it's a good idea running libuv-in-node before, just to be safe, as I only tested my fix on Linux.

[santigimeno]

CI: https://ci.nodejs.org/view/libuv/job/libuv-in-node/37/

[cjihrig]

Lots of infrastructure failures, but it looks like the platforms that actually ran the tests did pass.

[santigimeno]

CI: https://ci.nodejs.org/view/libuv/job/libuv-in-node/38/

[cjihrig]

That last CI run came back all green. I'm moving forward with the release and will include https://github.com/cjihrig/node-1/commit/574cd7700235d9650edf4fba910fe793aef62564 and https://github.com/santigimeno/node/commit/dcee84c43990eb5ace5c15a4385c1d4c9c4a1f8f in the Node update PR.

[skypjack]

Is it possible that the new features aren't documented yet? I tried to match the changelog with the online documentation and it looks like some things are missed. Am I wrong (it could be actually, it's 23.30 here)?

[cjihrig]

This is the changelog that was automatically generated for the release: https://github.com/libuv/libuv/blob/e4087dedf837f415056a45a838f639a3d9dc3ced/ChangeLog#L1-L95

You can compare that with https://github.com/libuv/libuv/compare/v1.20.3...v1.21.0 to see if anything is missing.

[skypjack]

Yeah, I know how to read a changelog since a while ago actually. Thank you anyway.
Mine was a kind way to say you that online documentation isn't up-to-date yet. :+1:

[bnoordhuis]

Can you be more precise?

[skypjack]

@bnoordhuis I mean that the online documentation isn't updated yet. As an example, here there are no info about UV_FS_LCHOWN, that is already documented in its .rst file. Do you want a full list of the missing parts?