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

Add support for HTTP2 and HTTP3 #918

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

pprkut
Copy link

@pprkut pprkut commented Dec 23, 2024

This adds generic support to make HTTP2 and HTTP3 requests and parse the responses correctly.

Pull Request Type

  • I have checked there is no other PR open for the same change.

This is a:

  • Bug fix
  • New feature
  • Documentation improvement
  • Code quality improvement

Context

The protocol_version config option now supports the values 2.0 and 3.0 to issue HTTP2 or HTTP3 requests respectively.

Detailed Description

Quality assurance

  • This change does NOT contain a breaking change (fix or feature that would cause existing functionality to change).
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added unit tests to accompany this PR.
  • The (new/existing) tests cover this PR 100%.
  • I have (manually) tested this code to the best of my abilities.
  • My code follows the style guidelines of this project.

Documentation

For new features:

  • I have added a code example showing how to use this feature to the examples directory.
  • I have added documentation about this feature to the docs directory.
    If the documentation is in a new markdown file, I have added a link to this new file to the Docs folder README.md file.

@schlessera
Copy link
Member

Hi @pprkut ,

Thanks for your work on this!

Right now, it is not in a mergeable state, as the testing that is included is not sufficient. The testing fails to verify that this solves the issue it is meant to solve, so it is unclear whether this actually makes HTTP2 and HTTP3 work. From what I've gathered so far, it only allows for the connected server to specify to use HTTP2 or HTTP3 without breaking early, but then it is unclear whether it will work or break on a bad protocol because there might still be pieces missing to actually get the communication to work flawlessly.

It also creates a discrepancy between the curl and fsockopen transports. While we can debate whether we should look into HTTP2/HTTP3 support for fsockopen, at the very least we should have a capability check in place to let a user of the library detect whether they can make use of them or not with the protocol(s) supported on the current platform.

Also, we'll need our test server (=> https://github.com/RequestsPHP/test-server) and our test proxies to allow for proper testing of these.

I'm happy to help work on this and move it forward, but I have to admit I'm not deep enough into the technical details at this point to know the full details of how we can set this up for proper testing. is this something you are able and willing to help with?

@pprkut
Copy link
Author

pprkut commented Feb 3, 2025

Thanks @schlessera for taking a look. I totally agree that there is more work left here. What I can say is that we needed this change to talk to Google's HTTP2 APIs and that seems to work (though I can't find concrete evidence in the logged headers that we're actually using HTTP2).

This is a critical feature for us, so I'll be happy to spend some more time on it to get it upstreamed 🙂

@pprkut
Copy link
Author

pprkut commented Feb 3, 2025

PHP's built-in web server seems to only support HTTP 1.1 (not even 1.0), based on https://github.com/php/php-src/blob/master/sapi/cli/php_cli_server.c#L719
I don't think we can fit HTTP2/3 support into the existing test server 😕

@schlessera
Copy link
Member

Thanks for the additional context, @pprkut, and I'm happy to hear you can invest some more time into this! 😊

Yes, so what we really need to get properly started is a strategy on how to test this reliably so we can say with certitude that the entire round-trip is fully working with HTTP2/HTTP3 - sending a header claiming it is supported is not enough.

Re. the existing test server, that is just one way of testing that we have available. Right now, we also have dockerized setup (in https://github.com/RequestsPHP/test-server/tree/feature/docker-setup branch) that is being run on https://render.com/ (=> https://requests-test-server.onrender.com/). And I'm happy to add additional infrastructure or services if that's what is needed to get reliable tests going. But I'm unsure what the best approach is here without more research.

Btw, just fetching https://requests-test-server.onrender.com/get from the browser seems to return it in HTTP/2.

@pprkut
Copy link
Author

pprkut commented Feb 5, 2025

There's CURLINFO_HTTP_VERSION which returns the HTTP version used for the last request, which should be certain enough I think. The downside is that that is only available with curl >= 7.50.0.
HTTP2 support was added in curl 7.33.0.

I made some sample requests to the test server you mentioned (as well as https://www.google.com) and it seems to support both HTTP2 and HTTP3 just fine. This is the test script I used:

<?php

$ch = curl_init();

curl_setopt($ch, CURLOPT_URL, 'https://requests-test-server.onrender.com/get');
// curl_setopt($ch, CURLOPT_URL, 'https://www.google.com');
curl_setopt($ch, CURLOPT_RETURNTRANSFER, TRUE);
curl_setopt($ch, CURLOPT_HTTP_VERSION, 31);

$page = curl_exec($ch);

echo "CURL_HTTP_VERSION_1_0: " . CURL_HTTP_VERSION_1_0 . "\n";
echo "CURL_HTTP_VERSION_1_1: " . CURL_HTTP_VERSION_1_1 . "\n";
echo "CURL_HTTP_VERSION_2: " . CURL_HTTP_VERSION_2 . "\n";
echo "CURL_HTTP_VERSION_2TLS: " . CURL_HTTP_VERSION_2TLS . "\n";
echo "CURL_HTTP_VERSION_2_0: " . CURL_HTTP_VERSION_2_0 . "\n";
echo "CURL_HTTP_VERSION_2_PRIOR_KNOWLEDGE: " . CURL_HTTP_VERSION_2_PRIOR_KNOWLEDGE . "\n";
echo "CURL_HTTP_VERSION_3: " . 30 . "\n";
echo "CURL_HTTP_VERSION_3ONLY: " . 31 . "\n";

$info = curl_getinfo($ch, CURLINFO_HTTP_VERSION);

var_dump($info);

curl_close($ch);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants