node icon indicating copy to clipboard operation
node copied to clipboard

http2: fix client async storage persistence

Open orgads opened this issue 1 year ago • 2 comments

Create and store an AsyncResource for each stream, following a similar approach as used in HttpAgent.

Fixes #55376

orgads avatar Oct 19 '24 20:10 orgads

Review requested:

  • [ ] @nodejs/http2
  • [ ] @nodejs/net

nodejs-github-bot avatar Oct 19 '24 20:10 nodejs-github-bot

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:

... and 33 files with indirect coverage changes

codecov[bot] avatar Oct 19 '24 22:10 codecov[bot]

I'll try to improve it and use internals instead of AsyncResource.

orgads avatar Oct 20 '24 05:10 orgads

I didn't find a better solution. Open for suggestions :)

orgads avatar Oct 20 '24 19:10 orgads

@mcollina Any comments?

orgads avatar Oct 21 '24 10:10 orgads

Are headers received only once per stream? If they are, we can unset the resource after using it in onSessionHeaders.

orgads avatar Oct 21 '24 13:10 orgads

Renamed asyncRes to reqAsync for consistency.

orgads avatar Oct 21 '24 15:10 orgads

CI: https://ci.nodejs.org/job/node-test-pull-request/63248/

nodejs-github-bot avatar Oct 22 '24 07:10 nodejs-github-bot

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?

orgads avatar Oct 22 '24 13:10 orgads

Rebased.

orgads avatar Oct 24 '24 19:10 orgads

@lpinca Can you please trigger CI again?

orgads avatar Oct 25 '24 11:10 orgads

CI: https://ci.nodejs.org/job/node-test-pull-request/63282/

nodejs-github-bot avatar Oct 25 '24 11:10 nodejs-github-bot

@lpinca the failures look random and unrelated to my changes. How do we proceed from here?

orgads avatar Oct 27 '24 04:10 orgads

CI: https://ci.nodejs.org/job/node-test-pull-request/63307/

nodejs-github-bot avatar Oct 27 '24 05:10 nodejs-github-bot

Passed! Thank you.

orgads avatar Oct 27 '24 07:10 orgads

How do we proceed from here?

orgads avatar Oct 29 '24 07:10 orgads

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/.ncu
https://github.com/nodejs/node/actions/runs/11594005797

nodejs-github-bot avatar Oct 30 '24 13:10 nodejs-github-bot

Landed in f67e45e0702baef1ee1253042e48fcb7b806d3b3

nodejs-github-bot avatar Oct 31 '24 17:10 nodejs-github-bot

Thank you all! What's the process for backporting it to 22.x?

orgads avatar Nov 01 '24 13:11 orgads

@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.

Flarna avatar Nov 04 '24 06:11 Flarna

Thank you.

orgads avatar Nov 04 '24 06:11 orgads