From b9b50f3193914149fbdcc0d93f15298327b4803c Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Tue, 24 Oct 2023 07:51:05 -0700 Subject: [PATCH] hyper: temporarily remove HTTP/2 support The current design of the Hyper integration requires rebuilding the Hyper clientconn for each request. However, building the clientconn requires resending the HTTP/2 connection preface, which is incorrect from a protocol perspective. That in turn causes servers to send GOAWAY frames, effectively degrading performance to "no connection reuse" in the best case. It may also be triggering some bugs where requests get dropped entirely and reconnects take too long. This doesn't rule out HTTP/2 support with Hyper, but it may take a redesign of the Hyper integration in order to make things work. Closes #12191 --- configure.ac | 5 ++--- docs/HYPER.md | 4 ++++ lib/c-hyper.c | 55 +++++++++++++++--------------------------------- lib/curl_setup.h | 3 ++- lib/version.c | 2 +- 5 files changed, 26 insertions(+), 43 deletions(-) diff --git a/configure.ac b/configure.ac index 83ad82231d..a2c5165034 100644 --- a/configure.ac +++ b/configure.ac @@ -174,7 +174,7 @@ curl_headers_msg="enabled (--disable-headers-api)" curl_ws_msg="no (--enable-websockets)" ssl_backends= curl_h1_msg="enabled (internal)" - curl_h2_msg="no (--with-nghttp2, --with-hyper)" + curl_h2_msg="no (--with-nghttp2)" curl_h3_msg="no (--with-ngtcp2 --with-nghttp3, --with-quiche, --with-msh3)" enable_altsvc="yes" @@ -803,7 +803,6 @@ if test X"$want_hyper" != Xno; then experimental="$experimental Hyper" AC_MSG_NOTICE([Hyper support is experimental]) curl_h1_msg="enabled (Hyper)" - curl_h2_msg=$curl_h1_msg HYPER_ENABLED=1 AC_DEFINE(USE_HYPER, 1, [if hyper is in use]) AC_SUBST(USE_HYPER, [1]) @@ -4576,7 +4575,7 @@ if test "x$USE_TLS_SRP" = "x1"; then SUPPORT_FEATURES="$SUPPORT_FEATURES TLS-SRP" fi -if test "x$USE_NGHTTP2" = "x1" -o "x$USE_HYPER" = "x1"; then +if test "x$USE_NGHTTP2" = "x1"; then SUPPORT_FEATURES="$SUPPORT_FEATURES HTTP2" fi diff --git a/docs/HYPER.md b/docs/HYPER.md index 1c3b0dded4..9932c1bbf7 100644 --- a/docs/HYPER.md +++ b/docs/HYPER.md @@ -57,6 +57,10 @@ The hyper backend does not support - leading whitespace in first HTTP/1 response header - HTTP/0.9 - HTTP/2 upgrade using HTTP:// URLs. Aka 'h2c' +- HTTP/2 in general. Hyper has support for HTTP/2 but the curl side + needs changes so that a `hyper_clientconn` can last for the duration + of a connection. Probably this means turning the Hyper HTTP/2 backend + into a connection filter. ## Remaining issues diff --git a/lib/c-hyper.c b/lib/c-hyper.c index f34c44cc73..e3f7d6d444 100644 --- a/lib/c-hyper.c +++ b/lib/c-hyper.c @@ -22,6 +22,10 @@ * ***************************************************************************/ +/* Curl's integration with Hyper. This replaces certain functions in http.c, + * based on configuration #defines. This implementation supports HTTP/1.1 but + * not HTTP/2. + */ #include "curl_setup.h" #if !defined(CURL_DISABLE_HTTP) && defined(USE_HYPER) @@ -539,11 +543,9 @@ CURLcode Curl_hyper_stream(struct Curl_easy *data, static CURLcode debug_request(struct Curl_easy *data, const char *method, - const char *path, - bool h2) + const char *path) { - char *req = aprintf("%s %s HTTP/%s\r\n", method, path, - h2?"2":"1.1"); + char *req = aprintf("%s %s HTTP/1.1\r\n", method, path); if(!req) return CURLE_OUT_OF_MEMORY; Curl_debug(data, CURLINFO_HEADER_OUT, req, strlen(req)); @@ -625,7 +627,6 @@ CURLcode Curl_hyper_header(struct Curl_easy *data, hyper_headers *headers, static CURLcode request_target(struct Curl_easy *data, struct connectdata *conn, const char *method, - bool h2, hyper_request *req) { CURLcode result; @@ -637,26 +638,13 @@ static CURLcode request_target(struct Curl_easy *data, if(result) return result; - if(h2 && hyper_request_set_uri_parts(req, - /* scheme */ - (uint8_t *)data->state.up.scheme, - strlen(data->state.up.scheme), - /* authority */ - (uint8_t *)conn->host.name, - strlen(conn->host.name), - /* path_and_query */ - (uint8_t *)Curl_dyn_uptr(&r), - Curl_dyn_len(&r))) { - failf(data, "error setting uri parts to hyper"); - result = CURLE_OUT_OF_MEMORY; - } - else if(!h2 && hyper_request_set_uri(req, (uint8_t *)Curl_dyn_uptr(&r), + if(hyper_request_set_uri(req, (uint8_t *)Curl_dyn_uptr(&r), Curl_dyn_len(&r))) { failf(data, "error setting uri to hyper"); result = CURLE_OUT_OF_MEMORY; } else - result = debug_request(data, method, Curl_dyn_ptr(&r), h2); + result = debug_request(data, method, Curl_dyn_ptr(&r)); Curl_dyn_free(&r); @@ -887,7 +875,6 @@ CURLcode Curl_http(struct Curl_easy *data, bool *done) const char *p_accept; /* Accept: string */ const char *method; Curl_HttpReq httpreq; - bool h2 = FALSE; const char *te = NULL; /* transfer-encoding */ hyper_code rc; @@ -960,8 +947,9 @@ CURLcode Curl_http(struct Curl_easy *data, bool *done) goto error; } if(conn->alpn == CURL_HTTP_VERSION_2) { - hyper_clientconn_options_http2(options, 1); - h2 = TRUE; + failf(data, "ALPN protocol h2 not supported with Hyper"); + result = CURLE_UNSUPPORTED_PROTOCOL; + goto error; } hyper_clientconn_options_set_preserve_header_case(options, 1); hyper_clientconn_options_set_preserve_header_order(options, 1); @@ -1012,7 +1000,7 @@ CURLcode Curl_http(struct Curl_easy *data, bool *done) } } else { - if(!h2 && !data->state.disableexpect) { + if(!data->state.disableexpect) { data->state.expect100header = TRUE; } } @@ -1023,7 +1011,7 @@ CURLcode Curl_http(struct Curl_easy *data, bool *done) goto error; } - result = request_target(data, conn, method, h2, req); + result = request_target(data, conn, method, req); if(result) goto error; @@ -1044,19 +1032,10 @@ CURLcode Curl_http(struct Curl_easy *data, bool *done) if(result) goto error; - if(!h2) { - if(data->state.aptr.host) { - result = Curl_hyper_header(data, headers, data->state.aptr.host); - if(result) - goto error; - } - } - else { - /* For HTTP/2, we show the Host: header as if we sent it, to make it look - like for HTTP/1 but it isn't actually sent since :authority is then - used. */ - Curl_debug(data, CURLINFO_HEADER_OUT, data->state.aptr.host, - strlen(data->state.aptr.host)); + if(data->state.aptr.host) { + result = Curl_hyper_header(data, headers, data->state.aptr.host); + if(result) + goto error; } if(data->state.aptr.proxyuserpwd) { diff --git a/lib/curl_setup.h b/lib/curl_setup.h index 10bb0d7f23..63320d73c1 100644 --- a/lib/curl_setup.h +++ b/lib/curl_setup.h @@ -761,7 +761,8 @@ int getpwuid_r(uid_t uid, struct passwd *pwd, char *buf, #define UNITTEST static #endif -#if defined(USE_NGHTTP2) || defined(USE_HYPER) +/* Hyper supports HTTP2 also, but Curl's integration with Hyper does not */ +#if defined(USE_NGHTTP2) #define USE_HTTP2 #endif diff --git a/lib/version.c b/lib/version.c index d1855313c2..175550b9fe 100644 --- a/lib/version.c +++ b/lib/version.c @@ -455,7 +455,7 @@ static const struct feat features_table[] = { #ifndef CURL_DISABLE_HSTS FEATURE("HSTS", NULL, CURL_VERSION_HSTS), #endif -#if defined(USE_NGHTTP2) || defined(USE_HYPER) +#if defined(USE_NGHTTP2) FEATURE("HTTP2", NULL, CURL_VERSION_HTTP2), #endif #if defined(ENABLE_QUIC) -- 2.47.3