CURLPIPE_MULTIPLEX tries pipelining even if "Upgrade: h2c" did not succeed [PATCH] #584

c0ff · 2015-12-28T16:47:05Z

* Found bundle for host XXX: 0x6e2ba23030
* Conn: 0 (0x6e2be80a70) Receive pipe weight: (-1/0), penalized: FALSE
* Found connection 0, with requests in the pipe (4)
* Re-using existing connection! (#0) with host XXX
> GET /file1 HTTP/1.1
Host: XXX
User-Agent: ZZZ/0.0 (Windows NT 6.2.9200; Win64; en-US) libcurl/7.47.0-DEV mbedTLS/1.3.14 zlib/1.2.8 nghttp2/1.6.1-DEV
Accept: */*
Connection: Upgrade, HTTP2-Settings
Upgrade: h2c
HTTP2-Settings: AAMAAABkAAQAAP__

< HTTP/1.1 200 OK
< Server: nginx/1.9.6
< Date: Mon, 28 Dec 2015 15:38:14 GMT
< Content-Type: text/plain
< Content-Length: 100601017
< Last-Modified: Thu, 13 Sep 2012 22:40:48 GMT
< Connection: keep-alive
< ETag: "505260f0-5ff0cb9"
< Accept-Ranges: bytes
< 
< HTTP/1.1 200 OK
< Server: nginx/1.9.6
< Date: Mon, 28 Dec 2015 15:38:39 GMT
< Content-Type: text/plain
< Content-Length: 135
< Last-Modified: Wed, 11 Dec 2013 13:04:31 GMT
< Connection: keep-alive
< ETag: "52a862df-87"
< Accept-Ranges: bytes
< 
* Found bundle for host XXX: 0x6e2ba23030
* Conn: 0 (0x6e2be80a70) Receive pipe weight: (-1/0), penalized: FALSE
* Found connection 0, with requests in the pipe (3)
* Re-using existing connection! (#0) with host XXX
> GET /file2 HTTP/1.1
Host: XXX
User-Agent: ZZZ/0.0 (Windows NT 6.2.9200; Win64; en-US) libcurl/7.47.0-DEV mbedTLS/1.3.14 zlib/1.2.8 nghttp2/1.6.1-DEV
Accept: */*
Connection: Upgrade, HTTP2-Settings
Upgrade: h2c
HTTP2-Settings: AAMAAABkAAQAAP__

* Found bundle for host XXX: 0x6e2ba23030
* Conn: 0 (0x6e2be80a70) Receive pipe weight: (-1/0), penalized: FALSE
* Found connection 0, with requests in the pipe (4)
* Re-using existing connection! (#0) with host XXX
> GET /file3 HTTP/1.1
Host: XXX
User-Agent: ZZZ/0.0 (Windows NT 6.2.9200; Win64; en-US) libcurl/7.47.0-DEV mbedTLS/1.3.14 zlib/1.2.8 nghttp2/1.6.1-DEV
Accept: */*
Connection: Upgrade, HTTP2-Settings
Upgrade: h2c
HTTP2-Settings: AAMAAABkAAQAAP__

< HTTP/1.1 200 OK
< Server: nginx/1.9.6
< Date: Mon, 28 Dec 2015 15:38:39 GMT
< Content-Type: text/plain
< Content-Length: 109
< Last-Modified: Thu, 13 Sep 2012 22:59:59 GMT
< Connection: keep-alive
< ETag: "5052656f-6d"
< Accept-Ranges: bytes
<

The text was updated successfully, but these errors were encountered:

c0ff · 2015-12-28T17:32:46Z

this seems to fix it:

diff --git "a/C:\\Users\\dsb\\AppData\\Local\\Temp\\TortoiseGit\\url2D80.tmp\\url-ddfa0d8-left.c" "b/C:\\libs\\curl\\lib\\url.c"
index be2f2f8..de36bad 100644
--- "a/C:\\Users\\dsb\\AppData\\Local\\Temp\\TortoiseGit\\url2D80.tmp\\url-ddfa0d8-left.c"
+++ "b/C:\\libs\\curl\\lib\\url.c"
@@ -2865,7 +2877,8 @@ static bool IsPipeliningPossible(const struct SessionHandle *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;
   }

bagder · 2016-01-05T11:10:15Z

Thanks!

bagder · 2016-01-08T13:41:11Z

This caused a regression (http://curl.haxx.se/mail/lib-2016-01/0031.html) so re-opened and we need to fix it differently.

bagder · 2016-01-10T00:05:30Z

The problem was not that it tried to multiplex, the problem is that it enables pipelining - although not asked to. I suggest this patch that seems to do the trick for me:

From 788e6a7018abc17bbfb49dac3a523f0f9f4818a4 Mon Sep 17 00:00:00 2001
From: Daniel Stenberg <daniel@haxx.se>
Date: Sun, 10 Jan 2016 01:00:06 +0100
Subject: [PATCH] ConnectionExists: only do pipelining/multiplexing when asked

When an HTTP/2 upgrade request fails (no protocol switch), it would
previously detect that as still possible to pipeline on (which is
acorrect) and do that when PIPEWAIT was enabled even if pipelining was
not explictily enabled.

It should only pipelined if explicitly asked to.

Closes #584
---
 lib/url.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/lib/url.c b/lib/url.c
index 75a1c44..3c91abb 100644
--- a/lib/url.c
+++ b/lib/url.c
@@ -3,11 +3,11 @@
  *  Project                     ___| | | |  _ \| |
  *                             / __| | | | |_) | |
  *                            | (__| |_| |  _ <| |___
  *                             \___|\___/|_| \_\_____|
  *
- * Copyright (C) 1998 - 2015, Daniel Stenberg, <daniel@haxx.se>, et al.
+ * Copyright (C) 1998 - 2016, Daniel Stenberg, <daniel@haxx.se>, et al.
  *
  * This software is licensed as described in the file COPYING, which
  * you should have received as part of this distribution. The terms
  * are also available at http://curl.haxx.se/docs/copyright.html.
  *
@@ -3151,12 +3151,16 @@ ConnectionExists(struct SessionHandle *data,
     size_t max_pipe_len = (bundle->multiuse != BUNDLE_MULTIPLEX)?
       max_pipeline_length(data->multi):0;
     size_t best_pipe_len = max_pipe_len;
     struct curl_llist_element *curr;

-    infof(data, "Found bundle for host %s: %p\n",
-          needle->host.name, (void *)bundle);
+    infof(data, "Found bundle for host %s: %p [%s]\n",
+          needle->host.name, (void *)bundle,
+          (bundle->multiuse== BUNDLE_PIPELINING?
+           "can pipeline":
+           (bundle->multiuse== BUNDLE_MULTIPLEX?
+            "can multiplex":"serially")));

     /* We can't pipe if we don't know anything about the server */
     if(canPipeline) {
       if(bundle->multiuse <= BUNDLE_UNKNOWN) {
         if((bundle->multiuse == BUNDLE_UNKNOWN) && data->set.pipewait) {
@@ -3166,10 +3170,21 @@ ConnectionExists(struct SessionHandle *data,
         }

         infof(data, "Server doesn't support multi-use (yet)\n");
         canPipeline = FALSE;
       }
+      if((bundle->multiuse == BUNDLE_PIPELINING) &&
+         !Curl_pipeline_wanted(data->multi, CURLPIPE_HTTP1)) {
+        /* not asked for, switch off */
+        infof(data, "Could pipeline, but not asked to!\n");
+        canPipeline = FALSE;
+      }
+      else if((bundle->multiuse == BUNDLE_MULTIPLEX) &&
+              !Curl_pipeline_wanted(data->multi, CURLPIPE_MULTIPLEX)) {
+        infof(data, "Could multiplex, but not asked to!\n");
+        canPipeline = FALSE;
+      }
     }

     curr = bundle->conn_list->head;
     while(curr) {
       bool match = FALSE;
-- 
2.7.0.rc3

c0ff · 2016-01-10T02:03:09Z

Oh, this one looks much better, although it goes beyond my current understanding of Curl internals.
Will try this patch for my situation in a couple of days.

Regards,
Dmitry.

On 10 янв. 2016 г., at 3:05, Daniel Stenberg notifications@github.com wrote:

The problem was not that it tried to multiplex, the problem is that it enables pipelining - although not asked to. I suggest this patch that seems to do the trick for me:

From 788e6a7018abc17bbfb49dac3a523f0f9f4818a4 Mon Sep 17 00:00:00 2001
From: Daniel Stenberg daniel@haxx.se
Date: Sun, 10 Jan 2016 01:00:06 +0100
Subject: [PATCH] ConnectionExists: only do pipelining/multiplexing when asked

When an HTTP/2 upgrade request fails (no protocol switch), it would
previously detect that as still possible to pipeline on (which is
acorrect) and do that when PIPEWAIT was enabled even if pipelining was
not explictily enabled.

It should only pipelined if explicitly asked to.

Closes #584

lib/url.c | 21 ++++++++++++++++++---
1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/lib/url.c b/lib/url.c
index 75a1c44..3c91abb 100644
--- a/lib/url.c
+++ b/lib/url.c
@@ -3,11 +3,11 @@
Project ___| | | | _ | |
                        / __| | | | |_) | |
                       | (**| |_| |  _ <| |_**
                        _**|_**/|_| \______|
* Copyright (C) 1998 - 2015, Daniel Stenberg, daniel@haxx.se, et al.

* Copyright (C) 1998 - 2016, Daniel Stenberg, daniel@haxx.se, et al.
*
This software is licensed as described in the file COPYING, which

you should have received as part of this distribution. The terms

are also available at http://curl.haxx.se/docs/copyright.html.
*
@@ -3151,12 +3151,16 @@ ConnectionExists(struct SessionHandle *data,
size_t max_pipe_len = (bundle->multiuse != BUNDLE_MULTIPLEX)?
max_pipeline_length(data->multi):0;
size_t best_pipe_len = max_pipe_len;
struct curl_llist_element *curr;

infof(data, "Found bundle for host %s: %p\n",
     needle->host.name, (void *)bundle);
infof(data, "Found bundle for host %s: %p [%s]\n",
     needle->host.name, (void *)bundle,
     (bundle->multiuse== BUNDLE_PIPELINING?
      "can pipeline":
      (bundle->multiuse== BUNDLE_MULTIPLEX?
       "can multiplex":"serially")));
/* We can't pipe if we don't know anything about the server */
if(canPipeline) {
if(bundle->multiuse <= BUNDLE_UNKNOWN) {
if((bundle->multiuse == BUNDLE_UNKNOWN) && data->set.pipewait) {
@@ -3166,10 +3170,21 @@ ConnectionExists(struct SessionHandle *data,
}
 infof(data, "Server doesn't support multi-use (yet)\n");
 canPipeline = FALSE;
}
 if((bundle->multiuse == BUNDLE_PIPELINING) &&
    !Curl_pipeline_wanted(data->multi, CURLPIPE_HTTP1)) {
   /\* not asked for, switch off */
   infof(data, "Could pipeline, but not asked to!\n");
   canPipeline = FALSE;
 }
 else if((bundle->multiuse == BUNDLE_MULTIPLEX) &&
         !Curl_pipeline_wanted(data->multi, CURLPIPE_MULTIPLEX)) {
   infof(data, "Could multiplex, but not asked to!\n");
   canPipeline = FALSE;
 }
}

curr = bundle->conn_list->head;
while(curr) {

bool match = FALSE;

2.7.0.rc3
—
Reply to this email directly or view it on GitHub.

bagder · 2016-01-11T22:56:26Z

I believe my fix is correct so I just merged that. Feel free to open a new bug or re-open this if you find it inadequate. Thanks!

c0ff · 2016-01-12T13:26:52Z

I confirm, your fix works perfectly.

c0ff changed the title ~~CURLPIPE_MULTIPLEX tries pipelining even if "Upgrade: h2c" did not succeed~~ CURLPIPE_MULTIPLEX tries pipelining even if "Upgrade: h2c" did not succeed [PATCH] Dec 30, 2015

bagder added the HTTP/2 label Jan 5, 2016

bagder self-assigned this Jan 5, 2016

bagder closed this as completed in 46cb70e Jan 5, 2016

bagder reopened this Jan 8, 2016

bagder closed this as completed in 13b6d3b Jan 11, 2016

stootill mentioned this issue May 12, 2017

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

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CURLPIPE_MULTIPLEX tries pipelining even if "Upgrade: h2c" did not succeed [PATCH] #584

CURLPIPE_MULTIPLEX tries pipelining even if "Upgrade: h2c" did not succeed [PATCH] #584

Closes #584

bool match = FALSE;

CURLPIPE_MULTIPLEX tries pipelining even if "Upgrade: h2c" did not succeed [PATCH] #584

CURLPIPE_MULTIPLEX tries pipelining even if "Upgrade: h2c" did not succeed [PATCH] #584

Comments

Closes #584

bool match = FALSE;