Connection is retried via HTTP/2 after HTTP_1_1_REQUIRED is received in some cases #6862
Comments
|
I haven't worked on reproducing this yet, but I could imagine a fix could maybe be done something like this? My idea is to make sure connection reuse isn't done if the existing connection uses HTTP version 2 or higher while the wanted HTTP version is lower than 2. diff --git a/lib/url.c b/lib/url.c
index 19fcfb842..e576a7eb9 100644
--- a/lib/url.c
+++ b/lib/url.c
@@ -1313,10 +1313,15 @@ ConnectionExists(struct Curl_easy *data,
/* one of them was different */
continue;
}
}
+ if((needle->handler->protocol & PROTO_FAMILY_HTTP) &&
+ (check->httpversion >= 20) &&
+ (data->state.httpwant < CURL_HTTP_VERSION_2_0))
+ continue;
+
if((needle->handler->flags&PROTOPT_SSL)
#ifndef CURL_DISABLE_PROXY
|| !needle->bits.httpproxy || needle->bits.tunnel_proxy
#endif
) { |
|
@ngg any chance you can try this? |
This patch is not working for me, the same reproduction case still retries it with HTTP/2. I'll try to find out why. |
|
I think that this new condition is ok, the connection reusing logic will work better this way, but it does not solve this exact problem because of another issue: It looks like the first retry doesn't have the connection downgraded, because after calling I think the ordering of these branches is wrong, probably checking |
|
I'm not sure I see how that can work if |
|
Is there a way to reproduce this issue against a public URL? Getting that thing up in a docker installation is a bit of a hurdle for me but if we can verify against something out there it would become a smaller effort. |
|
I started that docker image on a public server https://13.74.64.165. To make it easier to reproduce there are multiple large files hosted, named Somehow it seems easier to reproduce with 3 connections instead of 2 if the server is remote. As you can see, there are 3 connections to |
|
Thanks! With this I'll be able to get a better understanding of what's going on... |
bagder
added a commit
that referenced
this issue
Apr 18, 2021
The data_pending() function is otherwise designed to let the logic always drain the incoming socket before ending, but this is not a good idea when the currennt transfer (when it is a h2 stream) ends before the "drain" is done. Reported-by: Gergely Nagy Fixes #6862
bagder
added a commit
that referenced
this issue
Apr 19, 2021
The data_pending() function is otherwise designed to let the logic always drain the incoming socket before ending, but this is not a good idea when the currennt transfer (when it is a h2 stream) ends before the "drain" is done. Reported-by: Gergely Nagy Fixes #6862
|
Hi! I've tried with different number of connections, different file sizes and I found that it still does not work in some cases, but it might be caused by another bug, because it can happen both when the patches are applied and without them. It seems that in some cases, there is no retry at all after HTTP_1_1_REQUIRED is received, but unfortunately, I couldn't reproduce this with a remote server, only locally. With my original docker-based reproduction case, I've shrinked Around 1 times out of 4, the retry is missing. Output when there is (correctly) a retryOutput when the retry is missingWhen there is no retry, this line is output which does not appear when it's working correctly: |
|
After the new "fixup" commit (4fe8d00) it became worse, now the retries are sometimes happening over HTTP/2 again. Output when the retry is over HTTP/2 |
|
The initial patch broke ordinary HTTP tests. The second try wasn't done properly either since it re-used the |
|
The retry was and is (probably) still missing because curl fails to store return the correct reason back. If you can reproduce the problem, then an even more useful log of it would be to add diff --git a/lib/http2.c b/lib/http2.c
index ce9a0d393..faf87b4e3 100644
--- a/lib/http2.c
+++ b/lib/http2.c
@@ -56,10 +56,11 @@
#define NGHTTP2_HAS_SET_LOCAL_WINDOW_SIZE 1
#endif
#define HTTP2_HUGE_WINDOW_SIZE (32 * 1024 * 1024) /* 32 MB */
+#define DEBUG_HTTP2
#ifdef DEBUG_HTTP2
#define H2BUGF(x) x
#else
#define H2BUGF(x) do { } while(0)
#endif
daniel@storebror:~/src/curl [bagder/h1-reuse-not-h2]$ |
|
With the third version of the patch (6c4007e), it's still does the same as the second one. Here are the Output when the retry is correctly done via HTTP/1.1Output when the retry is done via HTTP/2Output when there is no retry |
|
Thanks for your patience. I kept banging on this and I got a better understanding of what happened, so the forth take is different but also feels like I better found the root cause and addressed it. It did reveal a rather embarrassing oversight in the h2 code as the stream error was stored per-connection and not per-stream which made it sensitive for race conditions. The PR has been updated. |
|
Thanks, this seems to solve this issue, now the retry seems to be always happening and always over HTTP/1.1. What's interesting to me (and probably it's one more bug) that I think this shouldn't work without the connection pool fix, but it does, the HTTP/2 connection is not reused. I think the stream error closes the whole HTTP/2 connection and that's why it's not reused, I don't think this should happen. I tried to find out why the connection is not reused, and it seems to me that sometimes it's fully closed before the retry looks for a connection, and sometimes it's skipped because the |
|
Yeah, that's a result of what I did in #6793 for #6788 - the entire connection is marked for closure after the Let me see if I can give that a stab as well while in here toying around... |
|
I pushed a removal of the forced closing of the connection. The want-http1.1 setting will then make ConnectionExists reject connections that are multiplexed because the new connection cannot be multiplexed, which then gets us the result and effect we want without providing that specific version check I originally proposed. |
|
The connection is still closed in |
|
Right, but that's for another reason. That too can be improved but then we're really going away from what this issue is about so I won't try that now in this PR. |
|
Alright, the fix in the PR looks good to me. I'd suggest to also merge the connection reuse fix from #6862 (comment) |
|
Why? it doesn't add anything. As I tried to explain, a request for a HTTP/1.1 connection can't get a HTTP/2 connection with the existing code. |
|
I think it's still possible to get a HTTP/2 connection that way, if a currently unused HTTP/2 connection is in the pool. I couldn't reproduce this case without modifying the curl tool, but it was easy to demonstrate it by disabling multiplexing diff --git a/src/tool_operate.c b/src/tool_operate.c
index 942643290..c308f5033 100644
--- a/src/tool_operate.c
+++ b/src/tool_operate.c
@@ -2268,6 +2268,7 @@ static CURLcode parallel_transfers(struct GlobalConfig *global,
multi = curl_multi_init();
if(!multi)
return CURLE_OUT_OF_MEMORY;
+ curl_multi_setopt(multi, CURLMOPT_PIPELINING, CURLPIPE_NOTHING);
result = add_parallel_transfers(global, multi, share,
&more_transfers, &added_transfers);Running the same command as before, the retry is done over HTTP/2 once again if the connection re-use logic is not patched. Patching the connection re-use fixes these cases as well. |
|
Ah... thanks for explaining. I'll take another look in the morning and probably proceed and get that patched merged as well. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I did this
We have a server, where some endpoints require TLS client cert authentication. As HTTP/2 does not support TLS renegotiation, when this endpoint is called, the server replies with
HTTP_1_1_REQUIREDand a new connection is necessary.In curl 7.76.0, there was a fix for the simplest case by closing the connection: #6793
However, if there is another multiplexed connection which uses the same HTTP/2 connection, this solution does not work.
To reproduce with an Apache server, use the following
Dockerfile:If this
Dockerfileis saved to an empty directory, run the following command in that directory to start the server:When the server is running, call curl with:
Around 50% of the time on my machine (but it's timing dependant, the size of the
largefilemight need to be modified in theDockerfileabove) the call is retried over HTTP/2.Click here for example curl output when it went wrong!
I expected the following
Curl should have retried the connection via HTTP/1.1, but it's retried with HTTP/2. It seems like that the HTTP/2 connection can stay in the pool (which is actually not a problem because further requests might be able to use that), and when that happen, the retry is done over HTTP/2. I think the proper solution would be to check the wanted HTTP version when choosing a connection to re-use.
curl/libcurl version
I've compiled curl from the 7.76 tag (https://github.com/curl/curl/tree/curl-7_76_0)
operating system
The text was updated successfully, but these errors were encountered: