Skip to content
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

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

Closed
c0ff opened this issue Dec 28, 2015 · 7 comments
Assignees
Labels

Comments

@c0ff
Copy link
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
Copy link
Contributor Author

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 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 bagder added the HTTP/2 label Jan 5, 2016
@bagder bagder self-assigned this Jan 5, 2016
@bagder
Copy link
Member

bagder commented Jan 5, 2016

Thanks!

@bagder bagder closed this as completed in 46cb70e Jan 5, 2016
@bagder
Copy link
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
Copy link
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
Copy link
Contributor Author

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 closed this as completed in 13b6d3b Jan 11, 2016
@bagder
Copy link
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
Copy link
Contributor Author

c0ff commented Jan 12, 2016

I confirm, your fix works perfectly.

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

No branches or pull requests

2 participants