Page MenuHomePhabricator
Create Task
Maniphest T13391

Recent versions of cURL default to HTTP/2, but HTTP/2 is newfangled poppycock
Closed, ResolvedPublic
Actions

Description

See https://discourse.phabricator-community.org/t/libcurl-defaults-to-http-2-but-doesnt-100-work-with-a-proxy-on-the-other-end/3062/.

Recent versions of cURL default to HTTP/2 where the server supports HTTP/2:

$ curl --verbose https://www.google.com/
...
> GET / HTTP/2

However, back in my day, we could type our HTTP requests by hand and we liked it that way. If serial transmission of messages was good enough for us, why should these new grads with their fancy degrees and their USB vape pen / fidget spinners doohickeys get anything different?

Also, libcurl just can't talk to HTTP/2 proxies, so if you hit an HTTP/2 proxy you're dead in the water, and all Google properties are reportedly a giant HTTP proxy that upgrades HTTP to HTTP/2 to QUIC-over-UDP to proprietary Google PWM signals running over proprietary Google wires on proprietary Google silicon.

https://github.com/curl/curl/issues/3570

It's unlikely we get anything by using HTTP/2 for any of our requests (generally, we are not limited by connections, and can resolve connection congestion effects in the rare cases where we may be by opening more connections), and unlikely that HTTP/1.1 is going away any time soon -- just as HTTP/1.0 still works fine everywhere today, over a thousand years after HTTP/1.1 was RFC'd. Support for HTTP/2 may be desirable some day but that day is likely far away.

The proposed patch is:

diff --git a/src/future/http/HTTPSFuture.php b/src/future/http/HTTPSFuture.php
index 0064a8f..9409d93 100644
--- a/src/future/http/HTTPSFuture.php
+++ b/src/future/http/HTTPSFuture.php
@@ -206,6 +206,7 @@ final class HTTPSFuture extends BaseHTTPFuture {
       curl_multi_add_handle(self::$multi, $curl);
 
       curl_setopt($curl, CURLOPT_URL, $uri);
+      curl_setopt($curl, CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_1_1);
 
       if (defined('CURLOPT_PROTOCOLS')) {
         // cURL supports a lot of protocols, and by default it will honor

Some version of this is likely reasonable, but I want to make sure CURL_HTTP_VERSION_1_1 is available in very old versions of cURL/PHP first.

Revisions and Commits

rPHU libphutil
D20744rPHU5060bdf9726b Force HTTP/1.1 in HTTPSFuture until HTTP/2 support stabilizes

Related Objects

Event Timeline

epriestley triaged this task as Normal priority.Aug 28 2019, 6:39 PM
epriestley created this task.
Herald added a subscriber: amckinley. · View Herald TranscriptAug 28 2019, 6:39 PM
epriestley added a comment.Aug 28 2019, 6:44 PM

A possible issue is that letting cURL pick a protocol might lead to it selecting HTTP/1.0 in some cases (how/when could it possibly do this? Only by hard-coding known-broken hostnames, I think?), and forcing it to use HTTP/1.1 could break those cases, so maybe I'll go spelunking here. I also can't immediately find a date of introduction for CURL_HTTP_VERSION_1_1 from the documentation.

epriestley added a comment.Aug 28 2019, 6:46 PM

According to curl/symbols-in-versions (this is a text file in the repository):

CURL_HTTP_VERSION_1_1           7.9.1

This version was released on a stone tablet circa 2001, so I think we're likely safe here.

epriestley added a comment.Aug 28 2019, 6:51 PM

A possible issue is that letting cURL pick a protocol might lead to it selecting HTTP/1.0 in some cases (how/when could it possibly do this?

At time of writing, cURL never does this. CURL_HTTP_VERSION_NONE has only two effects:

static bool use_http_1_1plus(const struct Curl_easy *data,
                             const struct connectdata *conn)
{

...

  return ((data->set.httpversion == CURL_HTTP_VERSION_NONE) ||
          (data->set.httpversion >= CURL_HTTP_VERSION_1_1));
}

...and:

#ifdef ENABLE_QUIC
    if(arg == CURL_HTTP_VERSION_3)
      ;
    else
#endif
#ifndef USE_NGHTTP2
    if(arg >= CURL_HTTP_VERSION_2)
      return CURLE_UNSUPPORTED_PROTOCOL;
#else
    if(arg > CURL_HTTP_VERSION_2_PRIOR_KNOWLEDGE)
      return CURLE_UNSUPPORTED_PROTOCOL;
    if(arg == CURL_HTTP_VERSION_NONE)
      arg = CURL_HTTP_VERSION_2TLS;
#endif

(How can using the preprocessor possibly be a good idea here?)

Anyway, I think we're in the clear to adopt the proposed patch verbatim.

epriestley added a revision: D20744: Force HTTP/1.1 in HTTPSFuture until HTTP/2 support stabilizes.Aug 28 2019, 6:55 PM
epriestley closed this task as Resolved by committing rPHU5060bdf9726b: Force HTTP/1.1 in HTTPSFuture until HTTP/2 support stabilizes.Aug 28 2019, 6:56 PM
epriestley claimed this task.
epriestley added a commit: rPHU5060bdf9726b: Force HTTP/1.1 in HTTPSFuture until HTTP/2 support stabilizes.
epriestley mentioned this in T13396: Support downloads of 2GB+ files with HTTPSFuture.Aug 29 2019, 4:33 PM
Phabricator · Built by Phacility