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.

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

Use HTTP/2 by default for TLS connections (for recent curl versions) #3249

Merged
merged 1 commit into from
Feb 22, 2023

Conversation

Mynacol
Copy link
Contributor

@Mynacol Mynacol commented Feb 8, 2023

This essentially reverts 492288d, as YouTube seems to have fixed their servers.
At least I was able to query the YouTube endpoint around 150 times with CURL_HTTP_VERSION_2TLS recently. They even advertise HTTP/3 support with an alt-svc HTTP header.

Setting CURL_HTTP_VERSION_2TLS explicitly leads to consistent behavior regardless of the curl version.
On the other hand that requires at least PHP 7.0.7 (we require PHP 7.4 already) and curl 7.47.0 [1], released on Jan 27 2016 [2]. Is this requirement a problem for this project?

Alternatively, we could set CURL_HTTP_VERSION_NONE and let curl decide on the version. This would support all curl versions and opens the possibility for HTTP/3, but leads again to inconsistent behavior depending on the underlying curl version.

I'm open to comments.

[1] https://www.php.net/manual/curl.constants.php
[2] https://curl.se/docs/releases.html

@dvikan
Copy link
Contributor

dvikan commented Feb 8, 2023

I'm a little confused by how it all interacts. I don't understand it all. I'm tempted to just merge it. What is the benefit? Is it that HTTP 2 is faster?

CURL_HTTP_VERSION_2TLS
Attempt HTTP 2 over TLS (HTTPS) only. libcurl will fall back to HTTP 1.1 if HTTP 2 cannot be negotiated with the HTTPS server. For clear text HTTP servers, libcurl will use 1.1. (Added in 7.47.0)

@Mynacol
Copy link
Contributor Author

Mynacol commented Feb 8, 2023 via email

@dvikan
Copy link
Contributor

dvikan commented Feb 9, 2023

Thanks for the explanation. I think I prefer let curl decide on the version. The prior bugfix applied to youtube but MAY have also affected other web servers.

This essentially reverts b042412,
as YouTube seems to have fixed their servers.
At least I was able to query the YouTube endpoint around 150 times with
CURL_HTTP_VERSION_2TLS recently. They even advertise HTTP/3 support with
an `alt-svc` HTTP header now.

This unsets CURLOPT_HTTP_VERSION to let curl decide
on the version. This would support all curl versions and opens the
possibility for HTTP/3, but leads to inconsistent behavior depending
on the underlying curl version.

We don't set CURL_HTTP_VERSION_NONE explicitly, as it is always the curl
default and opens the path to let individual bridges override the HTTP
version where necessary.

Alternatively, setting CURL_HTTP_VERSION_2TLS explicitly would lead to consistent behavior
regardless of the curl version, but might uncover old curl bugs before the
developers enabled HTTP/2 by default.
Additionally, that requires at least PHP 7.0.7 (we require PHP 7.4
already) and curl 7.47.0 [1], released on Jan 27 2016 [2].

See also the discussion on RSS-Bridge#3249

[1] https://www.php.net/manual/curl.constants.php
[2] https://curl.se/docs/releases.html
@Mynacol
Copy link
Contributor Author

Mynacol commented Feb 9, 2023

After some more thoughts I agree. Let curl decide on the version.
I updated the pull request and reworded the commit message accordingly.
I would say this is ready to merge. It runs with my bridges with no issues since 1.5 days.

Thanks for maintaining this project!

@Mynacol
Copy link
Contributor Author

Mynacol commented Feb 22, 2023

@dvikan I saw this PR is still open. Are there any pain points left? I thought it is ready to be merged.

BTW: This patch is running fine on my NixOS server since creating the PR.

@Mynacol Mynacol changed the title Use HTTP/2 by default for TLS connections Use HTTP/2 by default for TLS connections (for recent curl versions) Feb 22, 2023
@dvikan
Copy link
Contributor

dvikan commented Feb 22, 2023

Thanks for the ping. I'm just slow.

@dvikan dvikan merged commit 4450e9b into RSS-Bridge:master Feb 22, 2023
@Mynacol Mynacol deleted the http-2 branch February 22, 2023 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants