Upon HTTP_1_1_REQUIRED, retry the request with HTTP/1.1 #3349
Conversation
|
What do you think of this little extra dance to avoid the strstr in the error message? 0001-multi-polish-the-HTTP_1_1_REQUIRED-check.patch (also inlined here for easier browsing) From 2ed48bdb07fe3c5bc27a343126e68754bfd2267e Mon Sep 17 00:00:00 2001
From: Daniel Stenberg <daniel@haxx.se>
Date: Fri, 7 Dec 2018 17:22:29 +0100
Subject: [PATCH] multi: polish the HTTP_1_1_REQUIRED check
to avoid strstr() and depending of contents in the error message buffer
---
lib/http2.c | 8 ++++++++
lib/http2.h | 4 ++++
lib/multi.c | 5 +++--
3 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/lib/http2.c b/lib/http2.c
index c33bee3eb..a61d8c240 100644
--- a/lib/http2.c
+++ b/lib/http2.c
@@ -2365,10 +2365,18 @@ void Curl_http2_cleanup_dependencies(struct Curl_easy *data)
if(data->set.stream_depends_on)
Curl_http2_remove_child(data->set.stream_depends_on, data);
}
+/* Only call this function for a transfer that already got a HTTP/2
+ CURLE_HTTP2_STREAM error! */
+bool Curl_h2_http_1_1_error(struct connectdata *conn)
+{
+ struct http_conn *httpc = &conn->proto.httpc;
+ return (httpc->error_code == NGHTTP2_HTTP_1_1_REQUIRED);
+}
+
#else /* !USE_NGHTTP2 */
/* Satisfy external references even if http2 is not compiled in. */
#define CURL_DISABLE_TYPECHECK
diff --git a/lib/http2.h b/lib/http2.h
index 4492ec211..e1d2a79c7 100644
--- a/lib/http2.h
+++ b/lib/http2.h
@@ -57,10 +57,13 @@ CURLcode Curl_http2_add_child(struct Curl_easy *parent,
struct Curl_easy *child,
bool exclusive);
void Curl_http2_remove_child(struct Curl_easy *parent,
struct Curl_easy *child);
void Curl_http2_cleanup_dependencies(struct Curl_easy *data);
+
+/* returns true of the HTTP/2 stream error was HTTP_1_1_REQUIRED */
+bool Curl_h2_http_1_1_error(struct connectdata *conn);
#else /* USE_NGHTTP2 */
#define Curl_http2_init(x) CURLE_UNSUPPORTED_PROTOCOL
#define Curl_http2_send_request(x) CURLE_UNSUPPORTED_PROTOCOL
#define Curl_http2_request_upgrade(x,y) CURLE_UNSUPPORTED_PROTOCOL
#define Curl_http2_setup(x) CURLE_UNSUPPORTED_PROTOCOL
@@ -72,8 +75,9 @@ void Curl_http2_cleanup_dependencies(struct Curl_easy *data);
#define Curl_http2_done(x,y)
#define Curl_http2_done_sending(x)
#define Curl_http2_add_child(x, y, z)
#define Curl_http2_remove_child(x, y)
#define Curl_http2_cleanup_dependencies(x)
+#define Curl_h2_http_1_1_error(x) 0
#endif
#endif /* HEADER_CURL_HTTP2_H */
diff --git a/lib/multi.c b/lib/multi.c
index d36edfbd9..15fdff069 100644
--- a/lib/multi.c
+++ b/lib/multi.c
@@ -44,10 +44,11 @@
#include "pipeline.h"
#include "sigpipe.h"
#include "vtls/vtls.h"
#include "connect.h"
#include "http_proxy.h"
+#include "http2.h"
/* The last 3 #include files should be in this order */
#include "curl_printf.h"
#include "curl_memory.h"
#include "memdebug.h"
@@ -1952,12 +1953,12 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
/* if we are to retry, set the result to OK and consider the
request as done */
result = CURLE_OK;
done = TRUE;
}
- } else if(CURLE_HTTP2_STREAM == result && data->set.errorbuffer &&
- strstr(data->set.errorbuffer, "HTTP_1_1_REQUIRED")) {
+ } else if((CURLE_HTTP2_STREAM == result) &&
+ Curl_h2_http_1_1_error(data->easy_conn)) {
CURLcode ret = Curl_retry_request(data->easy_conn, &newurl);
infof(data, "Forcing HTTP/1.1 for NTLM");
data->set.httpversion = CURL_HTTP_VERSION_1_1;
--
2.19.2
|
Oops... ;-)
That looks very good, thank you! Will you make the changes and complete this PR? |
Actually, let me give it a try. |
This is a companion patch to cbea2fd (NTLM: force the connection to HTTP/1.1, 2018-12-06): with NTLM, we can switch to HTTP/1.1 preemptively. However, with other (Negotiate) authentication it is not clear to this developer whether there is a way to make it work with HTTP/2, so let's try HTTP/2 first and fall back in case we encounter the error HTTP_1_1_REQUIRED. Note: we will still keep the NTLM workaround, as it avoids an extra round trip. Daniel Stenberg helped a lot with this patch, in particular by suggesting to introduce the Curl_h2_http_1_1_error() function. Closes curl#3349 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
This is a companion patch to cbea2fd (NTLM: force the connection to HTTP/1.1, 2018-12-06): with NTLM, we can switch to HTTP/1.1 preemptively. However, with other (Negotiate) authentication it is not clear to this developer whether there is a way to make it work with HTTP/2, so let's try HTTP/2 first and fall back in case we encounter the error HTTP_1_1_REQUIRED. Note: we will still keep the NTLM workaround, as it avoids an extra round trip. Daniel Stenberg helped a lot with this patch, in particular by suggesting to introduce the Curl_h2_http_1_1_error() function. Closes #3349 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Done. And tested (again, reverting that NTLM patch and doing the |
NTLM is incompatible with HTTP/2, and most (or all) of Kerberos, too. In such cases, it is a problem that cURL v7.62.0 changed the default to use HTTP/2 for https:// connections whenever servers indicate support for it. This is a backport of curl/curl#3345 and curl/curl#3349, so that Git for Windows v2.20.0 will have those fixes. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Thanks! |
Thank you for your help, and I am really happy that this bug has been addressed (and that those support requests have been averted...)! |
This is a companion patch to cbea2fd (NTLM: force the connection to HTTP/1.1, 2018-12-06): with NTLM, we can switch to HTTP/1.1 preemptively. However, with other (Negotiate) authentication it is not clear to this developer whether there is a way to make it work with HTTP/2, so let's try HTTP/2 first and fall back in case we encounter the error HTTP_1_1_REQUIRED. Note: we will still keep the NTLM workaround, as it avoids an extra round trip. Daniel Stenberg helped a lot with this patch, in particular by suggesting to introduce the Curl_h2_http_1_1_error() function. Closes curl#3349 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
This is a companion patch to #3345 which switches to HTTP/1.1 preemptively when we are about to try NTLM authentication.
However, with other (Negotiate) authentication it is not clear to this developer whether there is a way to make it work with HTTP/2, so let's try HTTP/2 first and fall back in case we encounter the error HTTP_1_1_REQUIRED.
This patch was developed with a lot of help by Daniel Stenberg.
(And yes, I tested this, by reverting the NTLM patch and authenticating against the same test server which I used to test the NTLM patch.)