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.

Sign up for GitHub

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

Jump to bottom

Initial proposal for additional http2settings #49025

Merged
merged 3 commits into from
Dec 18, 2023
Merged

Initial proposal for additional http2settings #49025

merged 3 commits into from
Dec 18, 2023

Conversation

martenrichter
Copy link
Contributor

@martenrichter martenrichter commented Aug 5, 2023 •
edited
Loading

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

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http2
  • @nodejs/net

@nodejs-github-bot 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 martenrichter mentioned this pull request Aug 13, 2023
Closed
@martenrichter martenrichter force-pushed the addlsettings branch 5 times, most recently from 6b98fbb to 9b23524 Compare August 13, 2023 22:18
@martenrichter martenrichter mentioned this pull request Aug 16, 2023
Closed
@martenrichter martenrichter force-pushed the addlsettings branch 2 times, most recently from 0b52839 to 2bfb762 Compare August 21, 2023 07:15
@martenrichter
Copy link
Contributor Author

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

@martenrichter martenrichter marked this pull request as ready for review August 21, 2023 07:17
@mcollina
Copy link
Member

mcollina commented Sep 4, 2023

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

@martenrichter
Copy link
Contributor Author

martenrichter commented Sep 4, 2023 •
edited
Loading

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.

mcollina
mcollina reviewed Sep 4, 2023
View reviewed changes
doc/api/http2.md Outdated
in node and the underlying libraries. The key of the object define the
numeric ids of the settings and the values the numeric value of the settings.
This property is not supported for remote and local settings. It is only
supported for sending SETTINGS.
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be explained in greater detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

yes

@martenrichter
Copy link
Contributor Author

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

@marco-ippolito
Copy link
Member

you can lint with make lint-js-fix 😄

@martenrichter
Copy link
Contributor Author

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.

@martenrichter
Copy link
Contributor Author

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)

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 11, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 11, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Sep 12, 2023
Open
19 tasks
@martenrichter
Copy link
Contributor Author

Hopefully the last correction for CI to work.

@nodejs-github-bot
Copy link
Collaborator

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

@mcollina
Copy link
Member

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

@martenrichter
Copy link
Contributor Author

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

martenrichter and others added 3 commits November 27, 2023 07:02
@martenrichter
http2: addtl http/2 settings
2b53b16 
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
@martenrichter
http2: add tests for customSettings
6bedd4c 
Test for the http2 setting's custom settings were added.
@martenrichter @jasnell
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 <[email protected]>
@martenrichter
Copy link
Contributor Author

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.

@martenrichter
Copy link
Contributor Author

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.

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 27, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 27, 2023
@nodejs-github-bot
Copy link
Collaborator

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

marco-ippolito
marco-ippolito approved these changes Nov 27, 2023
View reviewed changes
Copy link
Member

@marco-ippolito marco-ippolito left a comment

Choose a reason for hiding this comment

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

LGTM

@martenrichter
Copy link
Contributor Author

martenrichter commented Nov 27, 2023 •
edited
Loading

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.

@nodejs-github-bot
Copy link
Collaborator

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

@devsnek devsnek added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 15, 2023
@github-actions 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
Copy link
Collaborator

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

@mcollina mcollina added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed needs-ci PRs that need a full CI run. labels Dec 18, 2023
@nodejs-github-bot 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 nodejs-github-bot merged commit 2c571c6 into nodejs:main Dec 18, 2023
66 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 2c571c6

@martenrichter martenrichter mentioned this pull request Dec 31, 2023
Merged
RafaelGSS pushed a commit that referenced this pull request Jan 2, 2024
@martenrichter @RafaelGSS
http2: addtl http/2 settings
89eee77 
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Jan 2, 2024
Merged
richardlau pushed a commit that referenced this pull request Mar 25, 2024
@martenrichter @richardlau
http2: addtl http/2 settings
23414a6 
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
@richardlau richardlau mentioned this pull request Mar 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marco-ippolito marco-ippolito marco-ippolito approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c Issues and PRs that require attention from people who are familiar with C . commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@martenrichter @nodejs-github-bot @mcollina @marco-ippolito @jasnell @devsnek