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

Enabling CURLPIPE_HTTP1 and CURLPIPE_MULTIPLEX, and requesting HTTP2 causes POST requests to get pipelined #1481

Closed
stootill opened this Issue May 12, 2017 · 3 comments

Comments

Projects
None yet
2 participants
@stootill

stootill commented May 12, 2017

I have setup libcurl to enable HTTP2, and am configuring multi to enable pipelining and multiplexing as follows:-

curl_multi_setopt(_curlMultiHandle, CURLMOPT_PIPELINING, CURLPIPE_HTTP1 | CURLPIPE_MULTIPLEX);

On each request I am requesting HTTP2 as follows:-

curl_easy_setopt(_curlHandle, CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_2_0);

When I run this in my app which is using a mixture of HTTP/1.1 and HTTP/2 endpoints, I see POST requests failing on calls to HTTP/1.1 endpoints. I've dug into the code and found that it seems that IsPipeliningPossible is ignoring the request method if the request asked for HTTP/2 upgrading and multiplexing is enabled. IsPipeliningPossible only checks the HTTP version that was asked for in the request, not the version that was negotiated in the connection when testing for multiplex support, so it seems that pipelining is wrongly being enabled for requests that do not have GET or HEAD methods on HTTP/1.1 channels.

I have fixed this locally by applying the following change:-

`
@@ -2902,7 +2902,8 @@ static bool IsPipeliningPossible(const struct Curl_easy *handle,
return TRUE;

 if(Curl_pipeline_wanted(handle->multi, CURLPIPE_MULTIPLEX) &&
  •   (handle->set.httpversion >= CURL_HTTP_VERSION_2))
    
  •   (handle->set.httpversion >= CURL_HTTP_VERSION_2) &&
    
  •   (conn->httpversion == 20))
     /* allows HTTP/2 */
     return TRUE;
    
    }
    `

but I've found other bug reports (#584) that suggest that this caused regressions elsewhere (https://curl.haxx.se/mail/lib-2016-01/0031.html) so am not sure how to proceed.

Thanks!

@bagder bagder added the HTTP label May 12, 2017

bagder added a commit that referenced this issue May 12, 2017

pipeline: fix mistakenly trying to pipeline POSTs
The function IsPipeliningPossible() would return TRUE if either
pipelining OR HTTP/2 were possible on a connection, which would lead to
it returning TRUE even for POSTs on HTTP/1 connections.

It now returns a bitmask so that the caller can differentiate which kind
the connection allows.

Fixes #1481
Reported-by: stootill at github
@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 12, 2017

Member

See #1483 for my attempted fix for this issue. Would be great if you could try it out!

Member

bagder commented May 12, 2017

See #1483 for my attempted fix for this issue. Would be great if you could try it out!

@stootill

This comment has been minimized.

Show comment
Hide comment
@stootill

stootill May 12, 2017

stootill commented May 12, 2017

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 12, 2017

Member

Excellent!

Member

bagder commented May 12, 2017

Excellent!

@bagder bagder closed this in 4cdb1be May 12, 2017

@lock lock bot locked as resolved and limited conversation to collaborators May 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.