Initial proposal for additional http2settings by martenrichter · Pull Request #49025 · nodejs/node · GitHub

This PR implements the ability to send additional HTTP/2 settings with node.js, which are currently not directly implemented with node and the underlying nghttp2 lib. Receiving these settings is not possible, since nghttp2 actively checls for and passes only settings, that it knows. It is currently limited to 10 additional settings.

The intention of this PR is first to get feedback for the used interfacing before progressing further with the PR.

TODOs:

Edit documentation (done)
Verify, please give me feedback, if I should add more precise verification, then check that the additional settings are present (got feedback).
Testing, I have not tested the code, I first like to get feedback. For automated tests, the problem is that receiving the settings is not supported by nghttp2. So probably have to get the settings frame and do a binary compare.
EDIT: Automated tests are added, and will be run soon.... (tested) EDIT2: Test run as part of node's test, which should suffcient.

Related issue
#48962

Review requested:

@nodejs/http2
@nodejs/net

The test now run through. So I think this one is ready for review.

Are those custom settings passed through the connection/other peer?

Are those custom settings passed through the connection/other peer?

As far as I understand the settings should be sent to the other side of the connection, so for example to the browser.
But nghttp2 only passes settings, that it knows down to the calling c++ code. So if the peer is also a node.js instance, it will not receive the custom settings, since nghttp2 does not pass these settings. But a browser, which expects these newer http settings will receive them.
So it is far from perfect, but the best one can do with the current interfaces provided by nghttp2.

I am currently separated from my development computer until the end of the week, I will then try to fix the problems the automated test showed.

This needs to be explained in greater detail.

This needs to be explained in greater detail.

So more like this:

The key of the object defines the numeric value of the settings type (as defined in the "HTTP/2 SETTINGS" registry established by [RFC7540]) and the values the actual numeric value of the settings.
 It is only supported for sending SETTINGS.
 Custom setiings are not supported for the functions retrieving remote and local settings as nghttp2 does not pass unknown HTTP/2 settings to node.js.

yes

Ok, I am back at my development machine.
I hope I have fixed all issues. For the js linting I am not so sure, since I have done everything by hand....

you can lint with make lint-js-fix 😄

you can lint with make lint-js-fix 😄

Yes, I know, but I checked out node at a commit where it was broken. And I did not find the time to recompile.
Interesting, the c++ linter is failing, I had run format-cpp.... so something went wrong.

I hopefully have fixed all failures by hand.
(Sorry, my setup is messed up: VS Code with docker on Windows with bind mount and suffering after some time input/output errors if I want to do more than this little patch, I would set it up again)

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

Hopefully the last correction for CI to work.

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

I think you pushed way more commits than you should have, there are a few spurious ones. Can you fix it?

Oh you are right. The rebasing went wrong......
Will take some time, I need to figure out, where to get them from backup.

Currently, node.js http/2 is limited in sending SETTINGs, that are currently implemented by nghttp2. However, nghttp2 has the ability to send arbitary SETTINGs, that are not known beforehand. This patch adds this feature including a fall back mechanism, if a SETTING is implemented in a later nghttp2 or node version. Fixes: nodejs#1337

Test for the http2 setting's custom settings were added.

Add an explanation to the documentation for Http2Settings to explain the usage of customSettings and its limitations. Co-authored-by: James M Snell <jasnell@gmail.com>

Ok, I have now force-pushed the branch, with cherry picking the changing commits. I have now to run tests to check, if liniting is ok.

Ok, make lint passed.
So it should be back to the state it should be.
I should not use the button in vs code/codespace for rebasing. Instead the command line creates better results.

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

LGTM

I have no idea, why some of the jobs fail. One fails with "parallel.test-inspector-break-when-eval", which seems to be unrelated to the changes. The other fails with out of memory.

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

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

Landed in 2c571c6

Currently, node.js http/2 is limited in sending SETTINGs, that are currently implemented by nghttp2. However, nghttp2 has the ability to send arbitary SETTINGs, that are not known beforehand. This patch adds this feature including a fall back mechanism, if a SETTING is implemented in a later nghttp2 or node version. Fixes: #1337 PR-URL: #49025 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>

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 Aug 5, 2023

martenrichter mentioned this pull request Aug 13, 2023

Problem with make jslint and thus make test #49130

Closed

martenrichter force-pushed the addlsettings branch 5 times, most recently from 6b98fbb to 9b23524 Compare August 13, 2023 22:18

martenrichter mentioned this pull request Aug 16, 2023

Define capsule protocol over WebSocket to enable use where HTTP/2 is not supported ietf-wg-webtrans/draft-ietf-webtrans-http2#90

Closed

martenrichter force-pushed the addlsettings branch 2 times, most recently from 0b52839 to 2bfb762 Compare August 21, 2023 07:15

martenrichter marked this pull request as ready for review August 21, 2023 07:17

mcollina reviewed Sep 4, 2023

View reviewed changes

martenrichter force-pushed the addlsettings branch from 2bfb762 to 67fc63b Compare September 7, 2023 17:05

martenrichter force-pushed the addlsettings branch from 67fc63b to f6b824e Compare September 9, 2023 10:24

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 11, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 11, 2023

github-actions Bot mentioned this pull request Sep 12, 2023

CI Reliability 2023-09-12 nodejs/reliability#663

Open

19 tasks

martenrichter force-pushed the addlsettings branch from f6b824e to f5f92d7 Compare September 12, 2023 06:13

martenrichter and others added 3 commits November 27, 2023 07:02

http2: add tests for customSettings

6bedd4c

Test for the http2 setting's custom settings were added.

http2: customSettings add documentation

2860c65

Add an explanation to the documentation for Http2Settings to explain the usage of customSettings and its limitations. Co-authored-by: James M Snell <jasnell@gmail.com>

martenrichter force-pushed the addlsettings branch from a6d13b9 to 2860c65 Compare November 27, 2023 07:22

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 27, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 27, 2023

marco-ippolito approved these changes Nov 27, 2023

View reviewed changes

devsnek added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 15, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 15, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 18, 2023

nodejs-github-bot merged commit 2c571c6 into nodejs:main Dec 18, 2023

martenrichter mentioned this pull request Dec 31, 2023

http2: Receive customsettings #51323

Merged

RafaelGSS mentioned this pull request Jan 2, 2024

v21.6.0 proposal #51342

Merged

richardlau mentioned this pull request Mar 25, 2024

v20.12.0 proposal #52212

Merged

Initial proposal for additional http2settings by martenrichter · Pull Request #49025 · nodejs/node · GitHub

Uh oh!

Uh oh!

Initial proposal for additional http2settings - #49025

Initial proposal for additional http2settings#49025nodejs-github-bot merged 3 commits intonodejs:mainnodejs/node:mainfrom martenrichter:addlsettingsmartenrichter/node:addlsettingsCopy head branch name to clipboard

Conversation

martenrichter commented Aug 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 5, 2023

Uh oh!

martenrichter commented Aug 21, 2023

Uh oh!

mcollina commented Sep 4, 2023

Uh oh!

martenrichter commented Sep 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina Sep 4, 2023

Choose a reason for hiding this comment

Uh oh!

martenrichter Sep 4, 2023

Choose a reason for hiding this comment

Uh oh!

mcollina Sep 4, 2023

Choose a reason for hiding this comment

Uh oh!

martenrichter commented Sep 7, 2023

Uh oh!

marco-ippolito commented Sep 9, 2023

Uh oh!

martenrichter commented Sep 9, 2023

Uh oh!

martenrichter commented Sep 9, 2023

Uh oh!

nodejs-github-bot commented Sep 11, 2023

Uh oh!

martenrichter commented Sep 12, 2023

Uh oh!

nodejs-github-bot commented Nov 26, 2023

Uh oh!

mcollina commented Nov 27, 2023

Uh oh!

martenrichter commented Nov 27, 2023

Uh oh!

martenrichter commented Nov 27, 2023

Uh oh!

martenrichter commented Nov 27, 2023

Uh oh!

nodejs-github-bot commented Nov 27, 2023

Uh oh!

marco-ippolito left a comment

Choose a reason for hiding this comment

Uh oh!

martenrichter commented Nov 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Dec 13, 2023

Uh oh!

nodejs-github-bot commented Dec 15, 2023

Uh oh!

nodejs-github-bot commented Dec 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Initial proposal for additional http2settings#49025
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
martenrichter:addlsettings

martenrichter commented Aug 5, 2023 •
edited
Loading

martenrichter commented Sep 4, 2023 •
edited
Loading

martenrichter commented Nov 27, 2023 •
edited
Loading