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

[v10.x] Update libuv to 1.27.0 #27728

Closed
wants to merge 221 commits into from

Conversation

MylesBorins
Copy link
Contributor

Seems to fix a bunch of problems. Any reason we shouldn't do this for 10.16.0?

addaleax and others added 30 commits April 17, 2019 11:32
Refs: nodejs#21093 (comment)

PR-URL: nodejs#21179
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#26604
Refs: nodejs#22892 (comment)
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Bartosz Sosnowski <[email protected]>
PR-URL: nodejs#26704
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Edit and condense the "What is LTS?" section of the Collaboroator Guide.

PR-URL: nodejs#26722
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
The How Does LTS Work section duplicates material in the release plan,
to which there is already a link in the doc. Unfortunately, it has gone
out of sync with the release plan, resulting in incorrect material being
in the Collaborator Guide. (The Release WG needs to approve certain
changes, not LTS WG as the guide currently says. It used to be the LTS
WG, but that changed.)

Instead of duplicating material in the Collaborator Guide and risking
that the two documents contradict each other again, instruct the reader
to refer to the release plan as the canonical source of information.

PR-URL: nodejs#26723
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Optimize test-http2-large-file so it only allocates a single buffer.

PR-URL: nodejs#26737
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Adrian Estrada <[email protected]>
PR-URL: nodejs#26830
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
PR-URL: nodejs#26585
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: nodejs#26838
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
PR-URL: nodejs#26720
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
We're hitting the Travis job timeout of 50mins if built with a new
compiler (as there is no ccache). Temporarily disable the running
of tests so the Travis job can complete within the timeout and
populate the ccache.

PR-URL: nodejs#26720
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Restore running tests on Travis once the ccache is populated.

PR-URL: nodejs#26720
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
test-console-stdio-setters needs to test against the global console in
order to test the setters for the lazy-loaded _stdout and _stderr
properties.

PR-URL: nodejs#26796
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
There is one condition in the `console.assert()` code that is not
tested currently. Add a test to confirm that `console.assert(false)`
does not include a `:` in its output.

PR-URL: nodejs#26827
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: nodejs#26857
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
* Rewrite some material for simplicity and directness.
* Remove outdated LTS material in the Collaborator Guide. (For example,
  refers to LTS WG for things that are now handled by the Release WG.)
* Minor change to text about force pushing (s/minimize/reduce/).

PR-URL: nodejs#26845
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Use `require('internal/util/inspect').format` instead of
`require('util').format`.

Refs: nodejs#26546

PR-URL: nodejs#26822
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Building on nodejs#23269, if multiple ICU download URLs are present, try the
next one in case of error.

Part of the ICU 63.1 bump, but independent code-wise.
nodejs#23244

PR-URL: nodejs#23715
Fixes: nodejs#22344
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
- Full release notes: http://site.icu-project.org/download/63

Fixes: nodejs#22344

PR-URL: nodejs#23715
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Use the same property name as http2 does to indicate that
the stream is in the state before the `ready` event is emitted.

PR-URL: nodejs#24067
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#24332
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Co-authored-by: Ivan Filenko <[email protected]>
Fixes: nodejs#18603
Refs: nodejs#18904
PR-URL: nodejs#23916
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
TLS client authentication should be tested, including failure scenarios.

PR-URL: nodejs#24733
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Remove the eslint-disable comments by using a strict comparison
instead of a Boolean cast.

PR-URL: nodejs#24995
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
This commit adds a space to the message that is displayed for tests that
are skipped when node was built --without-ssl. For example, this is what
is currently displayed:
"release test-https-agent-additional-optionsSkipping as node was
compiled without crypto support"

After this change this will be:
"release test-https-agent-additional-options: Skipping as node was
compiled without crypto support"

PR-URL: nodejs#25011
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Instead of doing it in the `internalBinding('async_wrap')`
initialization whose first call is uncertain depending on how
the native modules are loaded in JS land during bootstrap.

PR-URL: nodejs#25020
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: nodejs#25073
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#25078
Fixes: nodejs#24521
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
This makes sure all required flags are passed through to the test.
If that's not the case an error is thrown to inform the user what
flag is missing.

PR-URL: nodejs#24876
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
This simplifies the `longestSeqContainedIn()` logic by checking for
the first identical occurance of at least three frames instead of
the longest one.
It also removes an unused argument.

PR-URL: nodejs#24744
Reviewed-By: Matteo Collina <[email protected]>
richardlau and others added 11 commits May 16, 2019 02:40
If `file` is a file then on Windows `mkdir` on `file/a` returns an
`ENOENT` error while on POSIX the equivalent returns `ENOTDIR`. On the
POSIX systems `ENOTDIR` would break out of the loop but on Windows the
`ENOENT` would strip off the `a` and attempt to make `file` as a
directory. This would return `EEXIST` but the code wasn't detecting
that the existing path was a file and attempted to make `file/a` again.

PR-URL: nodejs#27207
Fixes: nodejs#27198
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Ben Coe <[email protected]>
Replace try-catch blocks in tests with `assert.rejects()` and
`assert.throws()`.

PR-URL: nodejs#27207
Fixes: nodejs#27198
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Ben Coe <[email protected]>
Libuv does not guarantee that handles have their close
callbacks called in the order in which they were added
(and in fact, currently calls them in reverse order).

This patch ensures that the `flush_signal_` handle
is no longer in use (i.e. its close callback has already
been run) when we signal to the main thread that
`~NodeTraceBuffer` may be destroyed.

The same applies for `~NodeTraceWriter`.

Credit for debugging goes to Gireesh Punathil.

Fixes: nodejs#25512
Co-authored-by: Gireesh Punathil <[email protected]>

PR-URL: nodejs#25896
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Eugene Ostroukhov <[email protected]>
This commit extracts code to create sockaddr which is shared by both
UDPWrap::DoBind and UDPWrap::DoSend.

PR-URL: nodejs#26070
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
PR-URL: nodejs#26898
Refs: nodejs#24928
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
On macOS, fs.copyFile() may not respect file permissions. There is a PR
for libuv that should fix this, but until it lands and we can either
float a patch or upgrade libuv, have a known_issues test.

Ref: nodejs#26936
Ref: libuv/libuv#2233

PR-URL: nodejs#26939
Refs: nodejs#26936
Refs: libuv/libuv#2233
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
PR-URL: nodejs#25571
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Notable changes:
- A bug that could result in 100% CPU utilization in Node
  has been fixed (libuv/libuv#2162)
- Node's report module will now include the entire Windows
  product name (libuv/libuv#2170)

PR-URL: nodejs#26037
Fixes: nodejs#26013
Fixes: nodejs#25875
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Starting in libuv 1.27.0, test-dgram-address.js should expect
EBADF instead of EINVAL.

PR-URL: nodejs#26707
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Notable changes:
- `statx()` is used to retrieve file birth times on
  supported platforms.
- Improved support of running under Windows safe mode.
- Add support for UDP connected sockets. Several functions
  can now return `UV_EBADF` instead of `UV_EINVAL`.
- SunOS support is improved.

PR-URL: nodejs#26707
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
For non-Windows platforms, the happy path behavior should be
identical. On Windows, uv_os_uname() attempts to use
RtlGetVersion() before falling back to the deprecated
GetVersionExW() that Node was previously using.

PR-URL: nodejs#25600
Reviewed-By: Bartosz Sosnowski <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@MylesBorins MylesBorins requested a review from cjihrig May 16, 2019 07:01
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot added libuv Issues and PRs related to the libuv dependency or the uv binding. v10.x labels May 16, 2019
@MylesBorins MylesBorins requested a review from addaleax May 16, 2019 07:01
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM but any reason not to upgrade to v1.28.0, the version of libuv that's in v12.2.0?

Of interest: libuv v1.29.0 was just released yesterday.

@MylesBorins
Copy link
Contributor Author

@bnoordhuis 1.28.0 has only been in a release for ~3 weeks where as 1.27.0 has for ~8. Seemed prudent. Are there important change in 1.28.0?

@nodejs-github-bot
Copy link
Collaborator

@MylesBorins
Copy link
Contributor Author

CI failures are unrelated...

landed in eef2deb...7b76acb

MylesBorins pushed a commit that referenced this pull request May 16, 2019
Backport-PR-URL: #27728
PR-URL: #25571
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
Notable changes:
- A bug that could result in 100% CPU utilization in Node
  has been fixed (libuv/libuv#2162)
- Node's report module will now include the entire Windows
  product name (libuv/libuv#2170)

Backport-PR-URL: #27728
PR-URL: #26037
Fixes: #26013
Fixes: #25875
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
Starting in libuv 1.27.0, test-dgram-address.js should expect
EBADF instead of EINVAL.

Backport-PR-URL: #27728
PR-URL: #26707
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
Notable changes:
- `statx()` is used to retrieve file birth times on
  supported platforms.
- Improved support of running under Windows safe mode.
- Add support for UDP connected sockets. Several functions
  can now return `UV_EBADF` instead of `UV_EINVAL`.
- SunOS support is improved.

Backport-PR-URL: #27728
PR-URL: #26707
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
For non-Windows platforms, the happy path behavior should be
identical. On Windows, uv_os_uname() attempts to use
RtlGetVersion() before falling back to the deprecated
GetVersionExW() that Node was previously using.

Backport-PR-URL: #27728
PR-URL: #25600
Reviewed-By: Bartosz Sosnowski <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@addaleax
Copy link
Member

@MylesBorins Re: 1.28.0, it seems relatively low-risk to me. It contains the fix for #26936, which seems like something that should end up in v10.x, but it also does not seem critical to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libuv Issues and PRs related to the libuv dependency or the uv binding.
Projects
None yet
Development

Successfully merging this pull request may close these issues.