feat(http2): expose `nghttp2_option_set_stream_reset_rate_limit` as a `http2.createServer` option #54875

arcanis · 2024-09-10T15:33:11Z

Due to how ESM works, Vite-like development servers on very large applications may have to perform thousands of requests to localhost. In some difficult to reproduce circumstances¹ the rate may be too elevated for a Node.js server which will then start answering with net::ERR_HTTP2_PROTOCOL_ERROR errors. This can be mitigated by increasing the rate limit.

In our case it required having browser extensions setup in specific ways which cause Chrome to deopt(?) part of the network transactions, messing with the timings just enough to trigger the issue. ↩

nodejs-github-bot · 2024-09-10T15:33:17Z

Review requested:

@nodejs/http
@nodejs/http2
@nodejs/net

github-actions · 2024-09-10T16:19:27Z

Failed to start CI

   ⚠  Something was pushed to the Pull Request branch since the last approving review.
   ✘  Refusing to run CI on potentially unsafe PR

https://github.com/nodejs/node/actions/runs/10796555755

codecov · 2024-09-10T17:54:19Z

Codecov Report

Attention: Patch coverage is 65.00000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 87.90%. Comparing base (03fe00e) to head (ae9c450).
Report is 338 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_http2.cc	0.00%	6 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54875      +/-   ##
==========================================
+ Coverage   87.60%   87.90%   +0.30%     
==========================================
  Files         650      651       +1     
  Lines      182829   183383     +554     
  Branches    35383    35717     +334     
==========================================
+ Hits       160166   161204    +1038     
+ Misses      15928    15462     -466     
+ Partials     6735     6717      -18

Files with missing lines	Coverage Δ
lib/internal/http2/util.js	`92.99% <100.00%> (+0.10%)`	⬆️
src/node_http2_state.h	`100.00% <ø> (ø)`
src/node_http2.cc	`85.05% <0.00%> (-0.30%)`	⬇️

... and 96 files with indirect coverage changes

arcanis · 2024-09-13T14:12:50Z

@jasnell I think the tests need to be requested again - I had to push another commit to fix the linting

nodejs-github-bot · 2024-09-21T15:46:53Z

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

arcanis · 2024-09-30T07:29:39Z

Is anything else needed from me? I think the failed tests are unrelated (and are expected to fail if I'm reading the output right?).

nodejs-github-bot · 2024-09-30T09:52:10Z

Landed in 6c6562c

PR-URL: #54875 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>

Notable changes: crypto: * (SEMVER-MINOR) add `KeyObject.prototype.toCryptoKey` (Filip Skokan) #55262 * (SEMVER-MINOR) add Date fields for `validTo` and `validFrom` (Andrew Moon) #54159 doc: * add abmusse to collaborators (Abdirahim Musse) #55086 http2: * (SEMVER-MINOR) expose `nghttp2_option_set_stream_reset_rate_limit` as an option (Maël Nison) #54875 lib: * (SEMVER-MINOR) propagate aborted state to dependent signals before firing events (jazelly) #54826 module: * (SEMVER-MINOR) support loading entrypoint as url (RedYetiDev) #54933 * (SEMVER-MINOR) implement the `"module-sync"` exports condition (Joyee Cheung) #54648 * (SEMVER-MINOR) implement `flushCompileCache()` (Joyee Cheung) #54971 * (SEMVER-MINOR) throw when invalid argument is passed to `enableCompileCache()` (Joyee Cheung) #54971 * (SEMVER-MINOR) write compile cache to temporary file and then rename it (Joyee Cheung) #54971 src: * mark node `--run` as stable (Yagiz Nizipli) #53763 test_runner: * (SEMVER-MINOR) support custom arguments in `run()` (Aviv Keller) #55126 * (SEMVER-MINOR) add `'test:summary'` event (Colin Ihrig) #54851 * (SEMVER-MINOR) add support for coverage via `run()` (Chemi Atlow) #53937 worker: * (SEMVER-MINOR) add `markAsUncloneable` api (Jason Zhang) #55234 PR-URL: TODO

Notable changes: crypto: * (SEMVER-MINOR) add `KeyObject.prototype.toCryptoKey` (Filip Skokan) #55262 * (SEMVER-MINOR) add Date fields for `validTo` and `validFrom` (Andrew Moon) #54159 doc: * add abmusse to collaborators (Abdirahim Musse) #55086 http2: * (SEMVER-MINOR) expose `nghttp2_option_set_stream_reset_rate_limit` as an option (Maël Nison) #54875 lib: * (SEMVER-MINOR) propagate aborted state to dependent signals before firing events (jazelly) #54826 module: * (SEMVER-MINOR) support loading entrypoint as url (RedYetiDev) #54933 * (SEMVER-MINOR) implement the `"module-sync"` exports condition (Joyee Cheung) #54648 * (SEMVER-MINOR) implement `flushCompileCache()` (Joyee Cheung) #54971 * (SEMVER-MINOR) throw when invalid argument is passed to `enableCompileCache()` (Joyee Cheung) #54971 * (SEMVER-MINOR) write compile cache to temporary file and then rename it (Joyee Cheung) #54971 src: * mark node `--run` as stable (Yagiz Nizipli) #53763 test_runner: * (SEMVER-MINOR) support custom arguments in `run()` (Aviv Keller) #55126 * (SEMVER-MINOR) add `'test:summary'` event (Colin Ihrig) #54851 * (SEMVER-MINOR) add support for coverage via `run()` (Chemi Atlow) #53937 worker: * (SEMVER-MINOR) add `markAsUncloneable` api (Jason Zhang) #55234 PR-URL: #55343

Notable changes: crypto: * (SEMVER-MINOR) add `KeyObject.prototype.toCryptoKey` (Filip Skokan) #55262 * (SEMVER-MINOR) add Date fields for `validTo` and `validFrom` (Andrew Moon) #54159 doc: * add abmusse to collaborators (Abdirahim Musse) #55086 http2: * (SEMVER-MINOR) expose `nghttp2_option_set_stream_reset_rate_limit` as an option (Maël Nison) #54875 lib: * (SEMVER-MINOR) propagate aborted state to dependent signals before firing events (jazelly) #54826 module: * (SEMVER-MINOR) support loading entrypoint as url (RedYetiDev) #54933 * (SEMVER-MINOR) implement the `"module-sync"` exports condition (Joyee Cheung) #54648 * (SEMVER-MINOR) implement `flushCompileCache()` (Joyee Cheung) #54971 * (SEMVER-MINOR) throw when invalid argument is passed to `enableCompileCache()` (Joyee Cheung) #54971 * (SEMVER-MINOR) write compile cache to temporary file and then rename it (Joyee Cheung) #54971 src: * mark `node --run` as stable (Yagiz Nizipli) #53763 test_runner: * (SEMVER-MINOR) support custom arguments in `run()` (Aviv Keller) #55126 * (SEMVER-MINOR) add `'test:summary'` event (Colin Ihrig) #54851 * (SEMVER-MINOR) add support for coverage via `run()` (Chemi Atlow) #53937 worker: * (SEMVER-MINOR) add `markAsUncloneable` api (Jason Zhang) #55234 PR-URL: #55343

Notable changes: crypto: * (SEMVER-MINOR) add `KeyObject.prototype.toCryptoKey` (Filip Skokan) #55262 * (SEMVER-MINOR) add Date fields for `validTo` and `validFrom` (Andrew Moon) #54159 doc: * add abmusse to collaborators (Abdirahim Musse) #55086 http2: * (SEMVER-MINOR) expose `nghttp2_option_set_stream_reset_rate_limit` as an option (Maël Nison) #54875 lib: * (SEMVER-MINOR) propagate aborted state to dependent signals before firing events (jazelly) #54826 module: * (SEMVER-MINOR) support loading entrypoint as url (RedYetiDev) #54933 * (SEMVER-MINOR) implement the `"module-sync"` exports condition (Joyee Cheung) #54648 * (SEMVER-MINOR) implement `flushCompileCache()` (Joyee Cheung) #54971 * (SEMVER-MINOR) throw when invalid argument is passed to `enableCompileCache()` (Joyee Cheung) #54971 * (SEMVER-MINOR) write compile cache to temporary file and then rename it (Joyee Cheung) #54971 process: * (SEMVER-MINOR) add `process.features.require_module` (Joyee Cheung) #55241 src: * mark `node --run` as stable (Yagiz Nizipli) #53763 test_runner: * (SEMVER-MINOR) support custom arguments in `run()` (Aviv Keller) #55126 * (SEMVER-MINOR) add `'test:summary'` event (Colin Ihrig) #54851 * (SEMVER-MINOR) add support for coverage via `run()` (Chemi Atlow) #53937 worker: * (SEMVER-MINOR) add `markAsUncloneable` api (Jason Zhang) #55234 PR-URL: #55343

Notable changes: crypto: * (SEMVER-MINOR) add `KeyObject.prototype.toCryptoKey` (Filip Skokan) #55262 * (SEMVER-MINOR) add Date fields for `validTo` and `validFrom` (Andrew Moon) #54159 doc: * add abmusse to collaborators (Abdirahim Musse) #55086 http2: * (SEMVER-MINOR) expose `nghttp2_option_set_stream_reset_rate_limit` as an option (Maël Nison) #54875 lib: * (SEMVER-MINOR) propagate aborted state to dependent signals before firing events (jazelly) #54826 module: * (SEMVER-MINOR) support loading entrypoint as url (RedYetiDev) #54933 * (SEMVER-MINOR) implement the `"module-sync"` exports condition (Joyee Cheung) #54648 * (SEMVER-MINOR) implement `flushCompileCache()` (Joyee Cheung) #54971 * (SEMVER-MINOR) throw when invalid argument is passed to `enableCompileCache()` (Joyee Cheung) #54971 * (SEMVER-MINOR) write compile cache to temporary file and then rename it (Joyee Cheung) #54971 process: * (SEMVER-MINOR) add `process.features.require_module` (Joyee Cheung) #55241 * (SEMVER-MINOR) add `process.features.typescript` (Aviv Keller) #54295 src: * mark `node --run` as stable (Yagiz Nizipli) #53763 test_runner: * (SEMVER-MINOR) support custom arguments in `run()` (Aviv Keller) #55126 * (SEMVER-MINOR) add `'test:summary'` event (Colin Ihrig) #54851 * (SEMVER-MINOR) add support for coverage via `run()` (Chemi Atlow) #53937 worker: * (SEMVER-MINOR) add `markAsUncloneable` api (Jason Zhang) #55234 PR-URL: #55343

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Sep 10, 2024

http2: expose nghttp2_option_set_stream_reset_rate_limit as an option

e543254

arcanis force-pushed the mael/stream-reset-rate-limit branch from cdaf73e to e543254 Compare September 10, 2024 15:35

remove debug

75a276c

jasnell approved these changes Sep 10, 2024

View reviewed changes

jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 10, 2024

format-md

ae9c450

github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Sep 10, 2024

jakecastelli added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Sep 13, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 13, 2024

This comment was marked as outdated.

Sign in to view

richardlau added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Sep 30, 2024

richardlau approved these changes Sep 30, 2024

View reviewed changes

richardlau added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 30, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 30, 2024

nodejs-github-bot merged commit 6c6562c into nodejs:main Sep 30, 2024
61 checks passed

richardlau added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 30, 2024

targos pushed a commit that referenced this pull request Oct 4, 2024

http2: expose nghttp2_option_set_stream_reset_rate_limit as an option

3caa109

PR-URL: #54875 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>

targos pushed a commit that referenced this pull request Oct 4, 2024

http2: expose nghttp2_option_set_stream_reset_rate_limit as an option

914db60

PR-URL: #54875 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>

aduh95 mentioned this pull request Oct 9, 2024

V22.10.0 proposal #55343

Open

RafaelGSS mentioned this pull request Oct 11, 2024

2024-10-16, Version 23.0.0 (Current) #55338

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(http2): expose `nghttp2_option_set_stream_reset_rate_limit` as a `http2.createServer` option #54875

feat(http2): expose `nghttp2_option_set_stream_reset_rate_limit` as a `http2.createServer` option #54875

arcanis commented Sep 10, 2024

nodejs-github-bot commented Sep 10, 2024

github-actions bot commented Sep 10, 2024

codecov bot commented Sep 10, 2024 •

edited

Loading

arcanis commented Sep 13, 2024

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Sep 21, 2024

arcanis commented Sep 30, 2024 •

edited

Loading

nodejs-github-bot commented Sep 30, 2024

feat(http2): expose nghttp2_option_set_stream_reset_rate_limit as a http2.createServer option #54875

feat(http2): expose nghttp2_option_set_stream_reset_rate_limit as a http2.createServer option #54875

Conversation

arcanis commented Sep 10, 2024

Footnotes

nodejs-github-bot commented Sep 10, 2024

github-actions bot commented Sep 10, 2024

codecov bot commented Sep 10, 2024 • edited Loading

Codecov Report

arcanis commented Sep 13, 2024

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Sep 21, 2024

arcanis commented Sep 30, 2024 • edited Loading

nodejs-github-bot commented Sep 30, 2024

feat(http2): expose `nghttp2_option_set_stream_reset_rate_limit` as a `http2.createServer` option #54875

feat(http2): expose `nghttp2_option_set_stream_reset_rate_limit` as a `http2.createServer` option #54875

codecov bot commented Sep 10, 2024 •

edited

Loading

arcanis commented Sep 30, 2024 •

edited

Loading