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

Closed
c0ff opened this Issue Dec 28, 2015 · 7 comments

Projects

None yet

2 participants

@c0ff
Contributor
c0ff commented Dec 28, 2015
* 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
< 
@c0ff
Contributor
c0ff commented Dec 28, 2015

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;
   }
@c0ff c0ff changed the title from CURLPIPE_MULTIPLEX tries pipelining even if "Upgrade: h2c" did not succeed to CURLPIPE_MULTIPLEX tries pipelining even if "Upgrade: h2c" did not succeed [PATCH] Dec 30, 2015
@bagder bagder added the HTTP/2 label Jan 5, 2016
@bagder bagder self-assigned this Jan 5, 2016
@bagder
Member
bagder commented Jan 5, 2016

Thanks!

@bagder bagder added a commit that closed this issue Jan 5, 2016
@bagder bagder multiplex: allow only once HTTP/2 is actually used
To make sure curl doesn't allow multiplexing before a connection is
upgraded to HTTP/2 (like when Upgrade: h2c fails), we must make sure the
connection uses HTTP/2 as well and not only check what's wanted.

Closes #584

Patch-by: c0ff
46cb70e
@bagder bagder closed this in 46cb70e Jan 5, 2016
@bagder
Member
bagder commented Jan 8, 2016

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 bagder reopened this Jan 8, 2016
@bagder
Member
bagder commented Jan 10, 2016

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
Contributor
c0ff commented Jan 10, 2016

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 bagder added a commit that closed this issue Jan 11, 2016
@bagder bagder 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
13b6d3b
@bagder bagder closed this in 13b6d3b Jan 11, 2016
@bagder
Member
bagder commented Jan 11, 2016

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
Contributor
c0ff commented Jan 12, 2016

I confirm, your fix works perfectly.

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