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

http2: use nghttp2_session_upgrade2 than nghttp2_session_upgrade #7041

Closed
wants to merge 1 commit into from
Closed

http2: use nghttp2_session_upgrade2 than nghttp2_session_upgrade #7041

wants to merge 1 commit into from

Conversation

starrify
Copy link
Contributor

I happen to notice from the upstream that nghttp2_session_upgrade may be considered deprecated:
https://github.com/nghttp2/nghttp2/blob/20079b4c2f688385ba9ecf723f958d0448894879/lib/includes/nghttp2/nghttp2.h#L3660-L3663

When searching for nghttp2_session_upgrade on GitHub, the only result returned is #521. Also there is no result when searching in the revision history. This may rule out the possibility that nghttp2_session_upgrade2 was once adapted but reverted due to some unexpected failure.

@starrify
http2: use nghttp2_session_upgrade2 than nghttp2_session_upgrade
7d8252b 
Following the upstream deprecation of nghttp2_session_upgrade.

Also provides further checks for requests with the HEAD method.
@bagder
Copy link
Member

bagder commented May 10, 2021

docs/INTERNALS.md says we support nghttp2 1.12.0 or later. Does this change then change our requirement? If so, maybe check for that function in the configure script to make sure a new enough lib is used?

@starrify
Copy link
Contributor Author

Thanks a lot @bagder for the insight and the immediate response! Also my apologies for not having checked that early enough.

It's confirmed that nghttp2_session_upgrade2 exists in nghttp2 v1.12.0: https://github.com/nghttp2/nghttp2/blob/v1.12.0/lib/includes/nghttp2/nghttp2.h#L3364

Being initially introduced in 269a10008, that function is observed to have been available since nghttp2 v1.5.0.

No check for nghttp2_session_upgrade2 is added yet due to the above observations, plus that there is no current check for nghttp2_session_upgrade.

@bagder
Copy link
Member

bagder commented May 11, 2021

We already have a check for a function that was added in 1.12.0 (nghttp2_session_set_local_window_size) - that should be enough to make sure that nghttp2 is at least of that version.

@bagder
Copy link
Member

bagder commented May 11, 2021

Thanks!

@bagder bagder closed this in 651a75e May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@starrify @bagder