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

v18.20.3 proposal #53028

Merged
merged 41 commits into from
May 21, 2024
Merged

v18.20.3 proposal #53028

merged 41 commits into from
May 21, 2024

Conversation

richardlau
Copy link
Member

@richardlau richardlau commented May 16, 2024

2024-05-21, Version 18.20.3 'Hydrogen' (LTS), @richardlau

Notable Changes

This release fixes a regression introduced in Node.js 18.19.0 where http.server.close() was incorrectly closing idle connections.

A fix has also been included for compiling Node.js from source with newer versions of Clang.

The list of keys used to sign releases has been synchronized with the current list from the main branch.

Updated dependencies

  • acorn updated to 8.11.3.
  • acorn-walk updated to 8.3.2.
  • ada updated to 2.7.8.
  • c-ares updated to 1.28.1.
  • corepack updated to 0.28.0.
  • nghttp2 updated to 1.61.0.
  • ngtcp2 updated to 1.3.0.
  • npm updated to 10.7.0. Includes a fix from [email protected] to limit the number of open connections npm/cli#7324.
  • simdutf updated to 5.2.4.
  • zlib updated to 1.3.0.1-motley-7d77fb7.

Commits


cc @nodejs/releasers

richardlau and others added 30 commits April 17, 2024 11:58
As a temporary measure to unblock the CI, skip the RSA implicit
rejection test when Node.js is built against a dynamically linked
OpenSSL.

PR-URL: #52542
Refs: #52537
Refs: nodejs-private/node-private#525
Refs: https://hackerone.com/reports/2269177
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Original commit message:

    Make bitfields only as wide as necessary for enums

    clang now complains when a BitField for an enum is too wide.
    We could suppress this, but it seems kind of useful from an
    uninformed distance, so I made a few bitfields smaller instead.

    (For AddressingMode, since its size is target-dependent, I added
    an explicit underlying type to the enum instead, which suppresses
    the diag on a per-enum basis.)

    This is without any understanding of the code I'm touching.
    Especially the change in v8-internal.h feels a bit risky to me.

    Bug: chromium:1348574
    Change-Id: I73395de593045036b72dadf4e3147b5f7e13c958
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3794708
    Commit-Queue: Nico Weber <[email protected]>
    Reviewed-by: Leszek Swirski <[email protected]>
    Reviewed-by: Hannes Payer <[email protected]>
    Auto-Submit: Nico Weber <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#82109}

Refs: v8/v8@d15d49b
PR-URL: #52337
Fixes: #52230
Reviewed-By: Rafael Gonzaga <[email protected]>
Correcting the #50194 backporting mistake.
closeIdleConnections shouldnot be called while server.close in node v18.
This behavior is for node v19 and above.

Fixes: #52330
Fixes: #51677
PR-URL: #52336
Refs: #50194
Reviewed-By: Nitzan Uziely <[email protected]>
Hard code the value of the host parameter to `common.localhostIPv4` in
`server.listen()` and `net.connect()`. This

1. ensures that the client `socket._handle` is not reinitialized during
   connection due to the family autodetection algorithm, preventing
   `parser.consume()` from being called with an invalid `socket._handle`
   parameter.
2. works around an issue in the FreeBSD 12 machine where the stress test
   is run where some sockets get stuck after connection.

PR-URL: #49574
Backport-PR-URL: #52384
Closes: #49565
Fixes: #49564
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
The maximum number of parsers in the free list is set to 1000. However
the test does not need to use this maximum. Reduce it to 50.

Refs: #50228 (comment)
PR-URL: #50240
Backport-PR-URL: #52384
Fixes: #49564
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
PR-URL: #52257
Refs: nodejs/Release#984
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
PR-URL: #50441
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: #51581
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
PR-URL: #51948
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
PR-URL: #52395
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
PR-URL: #51655
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Ulises Gascón <[email protected]>
PR-URL: #52138
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
PR-URL: #52381
Refs: #51670
Reviewed-By: Daniel Lemire <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
PR-URL: #52473
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
PR-URL: #52351
Reviewed-By: Luke Karrys <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Ulises Gascón <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: #52027
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Ulises Gascón <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: #52028
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Ulises Gascón <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
PR-URL: #50460
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: #50457
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
PR-URL: #50457
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
PR-URL: #51317
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: #51457
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
The thread-safe function's finalizer is not called in conjunction with
the garbage collection of a JS value. In fact, it keeps a strong
reference to the JS function it is expected to call. Thus, it is safe
to make calls that affect GC state from its body.

PR-URL: #51801
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
PR-URL: #52458
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
PR-URL: #51291
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
PR-URL: #51400
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
PR-URL: #51319
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: #51584
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: #51796
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
PR-URL: #52767
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@mcollina
Copy link
Member

This should be added if possible: #51290

legendecas and others added 2 commits May 16, 2024 22:20
At the point of `FreeEnvironment` and onwards, no JavaScript execution
associated with the Environment should be triggered.

Avoid draining platform tasks that can trigger JavaScript execution in
`FreeEnvironment`. The holder of `node::Environment` should immediately
call `node::MultiIsolatePlatform::UnregisterIsolate` and
`v8::Isolate::Dispose` to cancel pending foreground tasks and join
concurrent tasks after the environment was freed.

`NodePlatform` can properly handle the case in `RunForegroundTask` when
an Isolate out-lives its associated `node::Environment`.

PR-URL: #51290
Fixes: #47748
Fixes: #49344
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Notable changes:

This release fixes a regression introduced in Node.js 18.19.0 where
`http.server.close()` was incorrectly closing idle connections.

A fix has also been included for compiling Node.js from source with
newer versions of Clang.

The list of keys used to sign releases has been synchronized with the
current list from the `main` branch.

Updated dependencies:

- acorn updated to 8.11.3.
- acorn-walk updated to 8.3.2.
- ada updated to 2.7.8.
- c-ares updated to 1.28.1.
- corepack updated to 0.28.0.
- nghttp2 updated to 1.61.0.
- ngtcp2 updated to 1.3.0.
- npm updated to 10.7.0. Includes a fix from [email protected] to limit the
  number of open connections.
- simdutf updated to 5.2.4.
- zlib updated to 1.3.0.1-motley-7d77fb7.

PR-URL: #53028
@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label May 16, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 16, 2024
@nodejs-github-bot

This comment was marked as outdated.

@legendecas
Copy link
Member

@mcollina yeah, I can verify the problem can be reproduced on v18.x, and #51290 clean lands on the staging branch and fixes the problem.

@richardlau
Copy link
Member Author

@mcollina yeah, I can verify the problem can be reproduced on v18.x, and #51290 clean lands on the staging branch and fixes the problem.

I've already cherry-picked it 🙂.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 17, 2024

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@richardlau
Copy link
Member Author

richardlau commented May 17, 2024

$ ncu-ci citgm 3431 3433
--------------------------------------------------------------------------------
[1/1] Running CITGM: 3431
--------------------------------------------------------------------------------
✔  Summary data downloaded
✔  Results data downloaded
✔  Summary data downloaded
✔  Results data downloaded
----------------------------------- Summary ------------------------------------
Result     FAILURE
URL        https://ci.nodejs.org/job/citgm-smoker/3431/
Source     https://api.github.com/repos/nodejs/node/git/refs/tags/v18.20.2
Commit     [9aedf16f8fb3] 2024-04-10, Version 18.20.2 'Hydrogen' (LTS)
Date       2024-04-10 09:12:07 -0300
Author     Rafael Gonzaga <[email protected]>
----------------------------------- Summary ------------------------------------
Result     FAILURE
URL        https://ci.nodejs.org/job/citgm-smoker/3433/
Source     https://api.github.com/repos/nodejs/node/git/refs/heads/v18.20.3-proposal
Commit     [64903b1ca04d] 2024-05-21, Version 18.20.3 'Hydrogen' (LTS)
Date       2024-05-16 22:36:39 +0000
Author     Richard Lau <[email protected]>
----------------------------------- Results ------------------------------------



FAILURE: 5 failures in 3433 not present in 3431


┌────────────────────────┬───────────────────┬───────────────┐
│ (index)                │ 0                 │ 1             │
├────────────────────────┼───────────────────┼───────────────┤
│ alpine-last-latest-x64 │ 'pino-v9.1.0'     │               │
│ fedora-last-latest-x64 │                   │               │
│ debian11-x64           │ 'undici-v6.16.1'  │               │
│ rhel8-ppc64le          │ 'fastify-v4.27.0' │ 'tape-v5.7.5' │
│ ubuntu2204-64          │                   │               │
│ debian12-x64           │                   │               │
│ rhel8-x64              │                   │               │
│ aix72-ppc64            │                   │               │
│ rhel8-s390x            │ 'undici-v6.16.1'  │               │
│ fedora-latest-x64      │                   │               │
│ alpine-latest-x64      │                   │               │
└────────────────────────┴───────────────────┴───────────────┘

While that's not a lot of "new" failures, for undici-v6.16.1 Node.js 18.20.3 appears to be encountering a HeadersOverflowError on Linux:
e.g. https://ci.nodejs.org/job/citgm-smoker/3433/nodes=debian11-x64/testReport/junit/(root)/citgm/undici_v6_16_1/

  Content-Length is set when using a FormData body with fetch (3544.837845ms)
 /home/iojs/tmp/citgm_tmp/6ba972b9-9a01-41bc-bdcf-2022f809a1ae/undici/undici-fetch.js:12862
       Error.captureStackTrace(err, this);
             ^
 TypeError: fetch failed
     at /home/iojs/tmp/citgm_tmp/6ba972b9-9a01-41bc-bdcf-2022f809a1ae/undici/undici-fetch.js:12862:13 {
   [cause]: HeadersOverflowError: Headers Overflow Error
       at Parser.trackHeader (/home/iojs/tmp/citgm_tmp/6ba972b9-9a01-41bc-bdcf-2022f809a1ae/undici/undici-fetch.js:5749:37)
       at Parser.onHeaderField (/home/iojs/tmp/citgm_tmp/6ba972b9-9a01-41bc-bdcf-2022f809a1ae/undici/undici-fetch.js:5723:14)
       at wasm_on_header_field (/home/iojs/tmp/citgm_tmp/6ba972b9-9a01-41bc-bdcf-2022f809a1ae/undici/undici-fetch.js:5539:34)
       at wasm://wasm/0002f80e:wasm-function[41]:0x84cb
       at wasm://wasm/0002f80e:wasm-function[20]:0x605b
       at Parser.execute (/home/iojs/tmp/citgm_tmp/6ba972b9-9a01-41bc-bdcf-2022f809a1ae/undici/undici-fetch.js:5665:26)
       at Parser.readMore (/home/iojs/tmp/citgm_tmp/6ba972b9-9a01-41bc-bdcf-2022f809a1ae/undici/undici-fetch.js:5644:16)
       at TLSSocket.<anonymous> (/home/iojs/tmp/citgm_tmp/6ba972b9-9a01-41bc-bdcf-2022f809a1ae/undici/undici-fetch.js:5972:18)
       at TLSSocket.emit (node:events:517:28)
       at emitReadable_ (node:internal/streams/readable:633:12) {
     code: 'UND_ERR_HEADERS_OVERFLOW'
   }
 }
 Node.js v18.20.3

Also

(most of these don't show up in the comparison table because they failed with 18.20.2 for other reasons)

I can recreate this on my local Linux development machine (with undici 16.17.0, which I guess has just been released).
In current nodejs/citgm checkout:
Passes:

nvm run 18.20.2 bin/citgm undici

Fails:

cd /home/rlau/sandbox/github/trees/v18.x
configure --verbose && make -j 88
python3 tools/install.py install '' /home/rlau/sandbox/testnode
cd /home/rlau/sandbox/github/citgm
PATH=/home/rlau/sandbox/testnode/bin/:$PATH node bin/citgm undici

@richardlau
Copy link
Member Author

Bisecting points to #52336, which is meant to fix a regression (#51677 and #52330). @nodejs/undici thoughts?

@mcollina
Copy link
Member

We would need to investigate this. Fastify uses Undici internally, so I guess those things are connected.

@ronag @ShogunPanda could you take a look as well?

@metcoder95
Copy link
Member

Just to verify, I've been trying to isolate the issue and it seems is not strictly related to the one shared ([cause]: HeadersOverflowError: Headers Overflow Error), as that one its kind of expected and the tests passes ( is the result of an execSync).

For undici, the bisect lead me to what it seems the faulty: test/fetch/fetch-leak.js. It seems just a matter of closing the idle connections to make the test pass

@ShogunPanda
Copy link
Contributor

I agree with @metcoder95.
In the test he mentioned the failure was to to an t.after never completing as the HTTP server was not closed before the interval ran again.
If you swap the order of the t.after in that file the test would pass.
Anyway, to avoid differences between runtimes, I proposed a different patch, see: nodejs/undici#3279 (this patch also hides the misleading headers overflow output).

I think this failures are not related to node and thus we can proceed with this release.

@richardlau
Copy link
Member Author

I think this failures are not related to node and thus we can proceed with this release.

👍 Thanks @metcoder95 and @ShogunPanda for looking at this. I'll proceed with the release as planned tomorrow.

@mcollina
Copy link
Member

undici v6.18 is out, with a fix if you want to try again.

@richardlau
Copy link
Member Author

richardlau commented May 20, 2024

Release CI with 64903b1: https://ci-release.nodejs.org/job/iojs+release/10213/
Rebuild of platforms which failed to upload to the staging server (rhel8-arm64-release, rhel8-ppc64le-release and rhel8-s390x-release): https://ci-release.nodejs.org/job/iojs+release/10214/

@richardlau richardlau merged commit 64903b1 into v18.x May 21, 2024
55 checks passed
@richardlau richardlau deleted the v18.20.3-proposal branch May 21, 2024 11:54
richardlau added a commit that referenced this pull request May 21, 2024
richardlau added a commit that referenced this pull request May 21, 2024
Notable changes:

This release fixes a regression introduced in Node.js 18.19.0 where
`http.server.close()` was incorrectly closing idle connections.

A fix has also been included for compiling Node.js from source with
newer versions of Clang.

The list of keys used to sign releases has been synchronized with the
current list from the `main` branch.

Updated dependencies:

- acorn updated to 8.11.3.
- acorn-walk updated to 8.3.2.
- ada updated to 2.7.8.
- c-ares updated to 1.28.1.
- corepack updated to 0.28.0.
- nghttp2 updated to 1.61.0.
- ngtcp2 updated to 1.3.0.
- npm updated to 10.7.0. Includes a fix from [email protected] to limit the
  number of open connections.
- simdutf updated to 5.2.4.
- zlib updated to 1.3.0.1-motley-7d77fb7.

PR-URL: #53028
richardlau added a commit to nodejs/nodejs.org that referenced this pull request May 21, 2024
github-merge-queue bot pushed a commit to nodejs/nodejs.org that referenced this pull request May 21, 2024
github-merge-queue bot pushed a commit to nodejs/nodejs.org that referenced this pull request May 21, 2024
EliphazBouye pushed a commit to EliphazBouye/node that referenced this pull request Jun 20, 2024
Notable changes:

This release fixes a regression introduced in Node.js 18.19.0 where
`http.server.close()` was incorrectly closing idle connections.

A fix has also been included for compiling Node.js from source with
newer versions of Clang.

The list of keys used to sign releases has been synchronized with the
current list from the `main` branch.

Updated dependencies:

- acorn updated to 8.11.3.
- acorn-walk updated to 8.3.2.
- ada updated to 2.7.8.
- c-ares updated to 1.28.1.
- corepack updated to 0.28.0.
- nghttp2 updated to 1.61.0.
- ngtcp2 updated to 1.3.0.
- npm updated to 10.7.0. Includes a fix from [email protected] to limit the
  number of open connections.
- simdutf updated to 5.2.4.
- zlib updated to 1.3.0.1-motley-7d77fb7.

PR-URL: nodejs#53028
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
Notable changes:

This release fixes a regression introduced in Node.js 18.19.0 where
`http.server.close()` was incorrectly closing idle connections.

A fix has also been included for compiling Node.js from source with
newer versions of Clang.

The list of keys used to sign releases has been synchronized with the
current list from the `main` branch.

Updated dependencies:

- acorn updated to 8.11.3.
- acorn-walk updated to 8.3.2.
- ada updated to 2.7.8.
- c-ares updated to 1.28.1.
- corepack updated to 0.28.0.
- nghttp2 updated to 1.61.0.
- ngtcp2 updated to 1.3.0.
- npm updated to 10.7.0. Includes a fix from [email protected] to limit the
  number of open connections.
- simdutf updated to 5.2.4.
- zlib updated to 1.3.0.1-motley-7d77fb7.

PR-URL: nodejs#53028
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. dependencies Pull requests that update a dependency file. doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. needs-ci PRs that need a full CI run. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

node 18.20.0 build failure