http2: fix client async storage persistence
Create and store an AsyncResource for each stream, following a similar approach as used in HttpAgent.
Fixes #55376
Review requested:
- [ ] @nodejs/http2
- [ ] @nodejs/net
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 88.42%. Comparing base (
53b1050) to head (ecc25ca). Report is 34 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@
## main #55460 +/- ##
=======================================
Coverage 88.42% 88.42%
=======================================
Files 654 654
Lines 187552 187561 +9
Branches 36087 36090 +3
=======================================
+ Hits 165839 165854 +15
- Misses 14950 14956 +6
+ Partials 6763 6751 -12
| Files with missing lines | Coverage Δ | |
|---|---|---|
| lib/internal/http2/core.js | 95.53% <100.00%> (+0.01%) |
:arrow_up: |
I'll try to improve it and use internals instead of AsyncResource.
I didn't find a better solution. Open for suggestions :)
@mcollina Any comments?
Are headers received only once per stream? If they are, we can unset the resource after using it in onSessionHeaders.
Renamed asyncRes to reqAsync for consistency.
CI: https://ci.nodejs.org/job/node-test-pull-request/63248/
The failures are unrelated to my changes. https://ci.nodejs.org/job/node-test-commit-arm/nodes=rhel8-arm64/55481/console This one is unclear. I saw a similar timeout in another test on the previous run of the same configuration.
11:01:35 not ok 4121 parallel/test-stream-readable-unpipe-resume
11:01:35 ---
11:01:35 duration_ms: 360017.24500
11:01:35 severity: fail
11:01:35 exitcode: -15
11:01:35 stack: |-
11:01:35 timeout
11:01:35 ...
https://ci.nodejs.org/job/node-test-commit-osx-arm/17293/nodes=osx11/console I didn't touch http, only http2.
11:00:21 not ok 4213 sequential/test-http-server-request-timeouts-mixed
11:00:21 ---
11:00:21 duration_ms: 3218.23100
11:00:21 severity: fail
11:00:21 exitcode: 1
11:00:21 stack: |-
11:00:21 node:internal/assert/utils:281
11:00:21 throw err;
11:00:21 ^
11:00:21
11:00:21 AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:
11:00:21
11:00:21 assert(request2.completed)
11:00:21
11:00:21 at Timeout._onTimeout (/Users/iojs/build/workspace/node-test-commit-osx-arm/nodes/osx11/test/sequential/test-http-server-request-timeouts-mixed.js:108:5)
11:00:21 at listOnTimeout (node:internal/timers:614:17)
11:00:21 at process.processTimers (node:internal/timers:549:7) {
11:00:21 generatedMessage: true,
11:00:21 code: 'ERR_ASSERTION',
11:00:21 actual: false,
11:00:21 expected: true,
11:00:21 operator: '=='
11:00:21 }
Is it possible to retrigger these?
Rebased.
@lpinca Can you please trigger CI again?
CI: https://ci.nodejs.org/job/node-test-pull-request/63282/
@lpinca the failures look random and unrelated to my changes. How do we proceed from here?
CI: https://ci.nodejs.org/job/node-test-pull-request/63307/
Passed! Thank you.
How do we proceed from here?
Commit Queue failed
- Loading data for nodejs/node/pull/55460 ✔ Done loading data for nodejs/node/pull/55460 ----------------------------------- PR info ------------------------------------ Title http2: fix client async storage persistence (#55460) Author Orgad Shaneh <[email protected]> (@orgads) Branch orgads:http2-async -> nodejs:main Labels http2, needs-ci Commits 1 - http2: fix client async storage persistence Committers 1 - Orgad Shaneh <[email protected]> PR-URL: https://github.com/nodejs/node/pull/55460 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Matteo Collina <[email protected]> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/55460 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Matteo Collina <[email protected]> -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last approving review: ⚠ - http2: fix client async storage persistence ℹ This PR was created on Sat, 19 Oct 2024 20:46:33 GMT ✔ Approvals: 3 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/55460#pullrequestreview-2380687991 ✔ - Stephen Belanger (@Qard): https://github.com/nodejs/node/pull/55460#pullrequestreview-2384391523 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/55460#pullrequestreview-2382123844 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2024-10-27T05:45:51Z: https://ci.nodejs.org/job/node-test-pull-request/63307/ - Querying data for job/node-test-pull-request/63307/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/11594005797
Landed in f67e45e0702baef1ee1253042e48fcb7b806d3b3
Thank you all! What's the process for backporting it to 22.x?
@orgads Usually this goes automatically. First it will land on 23 and later on 22.
In case cherry-picking this commit from main to the release branch fails for some reason a label + comment will be added here indicating it requires a dedicated manual created backport PR.
Thank you.