Http2 multi stream, one of stream error cause Pipe broke #941

Closed
benghopy opened this Issue Aug 2, 2016 · 14 comments

Projects

None yet

3 participants

@benghopy
benghopy commented Aug 2, 2016

I did this

I add two easyhandle(one of them make http2 post requset, the ohter make http2 get request) to a multihandle, i already set CURLMOPT_MAX_HOST_CONNECTIONS and CURLMOPT_PIPELINING to reused the connction, if one of the easyhandle request failed because of timeout or readcallback abort, then cause "Pipe broke" and the connection will reconnect.

I expected the following

curl/libcurl version

curl --version
curl 7.46.0 (mipsel-unknown-linux-gnu) libcurl/7.46.0 OpenSSL/1.0.2e zlib/1.2.3 nghttp2/1.9.0
Protocols: http https smb smbs
Features: Largefile NTLM SSL libz HTTP2 UnixSockets

operating system

linux

@jay jay added the HTTP/2 label Aug 2, 2016
@jay
Member
jay commented Aug 2, 2016

I'm having trouble understanding the problem here. Can you try the latest version 7.50?

@benghopy
benghopy commented Aug 2, 2016

I mean that i want to reuse the connection to send the HTTP/2 request, then I used multi handle and add two easyhandle to make request, but one of request finished by tomeout and cause the reused connect broken. I find the code in curl: It close the ssl connection, and cause the other request broken too.
I just have already try about 7.47.1 and 7.49, but all of them have this problem.

@bagder
Member
bagder commented Aug 2, 2016

Can you please provide a minimalist example source code that reproduces the problem?

@benghopy
benghopy commented Aug 2, 2016 edited
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

/* somewhat unix-specific */
#include <sys/time.h>
#include <unistd.h>

/* curl stuff */
#include <curl/curl.h>

#ifndef CURLPIPE_MULTIPLEX
/* This little trick will just make sure that we don't enable pipelining for
   libcurls old enough to not have this symbol. It is _not_ defined to zero in
   a recent libcurl header. */
#define CURLPIPE_MULTIPLEX 0
#endif

#define NUM_HANDLES 50

void *curl_hnd[NUM_HANDLES];
int num_transfers;
int request_stop = 0;

static int
progress_cb(void *clientp,
                            curl_off_t dltotal,
                            curl_off_t dlnow,
                            curl_off_t ultotal,
                            curl_off_t ulnow)
{
    if (request_stop) {
        // If I return > 0, the libcurl will reconnect the connect
        printf("progress_callback exit\n");
        return 1;
    }

    return 0;
}

static void setup(CURL *hnd, int num)
{
  FILE *out;
  char filename[128];

  snprintf(filename, 128, "dl-%d", num);

  out = fopen(filename, "wb");

  /* write to this file */
  curl_easy_setopt(hnd, CURLOPT_WRITEDATA, out);

  /* set the same URL */
  curl_easy_setopt(hnd, CURLOPT_URL, "https://localhost:8443/index.html");

  /* send it verbose for max debuggaility */
  curl_easy_setopt(hnd, CURLOPT_VERBOSE, 1L);

  /* HTTP/2 please */
  curl_easy_setopt(hnd, CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_2_0);

  // Set the timeout val as mini as you can
  curl_easy_setopt(hnd, CURLOPT_TIMEOUT, 1L);

  curl_easy_setopt(hnd, CURLOPT_NOPROGRESS, 0);
  curl_easy_setopt(hnd, CURLOPT_XFERINFOFUNCTION, progress_cb);

  /* we use a self-signed test server, skip verification during debugging */
  curl_easy_setopt(hnd, CURLOPT_SSL_VERIFYPEER, 0L);
  curl_easy_setopt(hnd, CURLOPT_SSL_VERIFYHOST, 0L);

#if (CURLPIPE_MULTIPLEX > 0)
  /* wait for pipe connection to confirm */
  curl_easy_setopt(hnd, CURLOPT_PIPEWAIT, 1L);
#endif

  curl_hnd[num] = hnd;
}

/*
 * Simply download two files over HTTP/2, using the same physical connection!
 */
int main(int argc, char **argv)
{
  CURL *easy[NUM_HANDLES];
  CURLM *multi_handle;
  int i;
  int still_running; /* keep number of running handles */

  if(argc > 1)
    /* if given a number, do that many transfers */
    num_transfers = atoi(argv[1]);

  if(!num_transfers || (num_transfers > NUM_HANDLES))
    num_transfers = 3; /* a suitable low default */

  /* init a multi stack */
  multi_handle = curl_multi_init();

  for(i=0; i<num_transfers; i++) {
    easy[i] = curl_easy_init();
    /* set options */
    setup(easy[i], i);

    /* add the individual transfer */
    curl_multi_add_handle(multi_handle, easy[i]);
  }

  curl_multi_setopt(multi_handle, CURLMOPT_PIPELINING, CURLPIPE_MULTIPLEX);
  curl_multi_setopt(multi_handle, CURLMOPT_MAX_HOST_CONNECTIONS, 1L);
  /* we start some action by calling perform right away */
  curl_multi_perform(multi_handle, &still_running);

  do {
    struct timeval timeout;
    int rc; /* select() return code */
    CURLMcode mc; /* curl_multi_fdset() return code */

    fd_set fdread;
    fd_set fdwrite;
    fd_set fdexcep;
    int maxfd = -1;

    long curl_timeo = -1;

    FD_ZERO(&fdread);
    FD_ZERO(&fdwrite);
    FD_ZERO(&fdexcep);

    /* set a suitable timeout to play around with */
    timeout.tv_sec = 1;
    timeout.tv_usec = 0;

    curl_multi_timeout(multi_handle, &curl_timeo);
    if(curl_timeo >= 0) {
      timeout.tv_sec = curl_timeo / 1000;
      if(timeout.tv_sec > 1)
        timeout.tv_sec = 1;
      else
        timeout.tv_usec = (curl_timeo % 1000) * 1000;
    }

    /* get file descriptors from the transfers */
    mc = curl_multi_fdset(multi_handle, &fdread, &fdwrite, &fdexcep, &maxfd);

    if(mc != CURLM_OK) {
      fprintf(stderr, "curl_multi_fdset() failed, code %d.\n", mc);
      break;
    }

    /* On success the value of maxfd is guaranteed to be >= -1. We call
       select(maxfd + 1, ...); specially in case of (maxfd == -1) there are
       no fds ready yet so we call select(0, ...) --or Sleep() on Windows--
       to sleep 100ms, which is the minimum suggested value in the
       curl_multi_fdset() doc. */

    if(maxfd == -1) {
#ifdef _WIN32
      Sleep(100);
      rc = 0;
#else
      /* Portable sleep for platforms other than Windows. */
      struct timeval wait = { 0, 100 * 1000 }; /* 100ms */
      rc = select(0, NULL, NULL, NULL, &wait);
#endif
    }
    else {
      /* Note that on some platforms 'timeout' may be modified by select().
         If you need access to the original value save a copy beforehand. */
      rc = select(maxfd+1, &fdread, &fdwrite, &fdexcep, &timeout);
    }

    switch(rc) {
    case -1:
      /* select error */
      break;
    case 0:
    default:
      /* timeout or readable/writable sockets */
      curl_multi_perform(multi_handle, &still_running);
      break;
    }
  } while(still_running);

  curl_multi_cleanup(multi_handle);

  for(i=0; i<num_transfers; i++)
    curl_easy_cleanup(easy[i]);

  return 0;
}
That is my sample code to test, I donot have a http2 server to have a test.
@benghopy
benghopy commented Aug 2, 2016

muilt c

@benghopy
benghopy commented Aug 3, 2016

Is that a problem of curl or that my way to used curl incorrect???

@bagder
Member
bagder commented Aug 3, 2016

It looks like a problem in curl.

@benghopy
benghopy commented Aug 4, 2016

Is there any way to fix this?

@bagder
Member
bagder commented Aug 4, 2016

Sure, first we figure out what the problem is exactly, then we change the code to make sure it works for this case as well. Then we land the change, ideally together with a new test or two.

@benghopy
benghopy commented Aug 10, 2016 edited

Hello bagder:
Does tihs problem will fix at the next version of curl?

@bagder
Member
bagder commented Aug 10, 2016

Assuming that we have a fix before that is to be released, sure. I've verified that it is easy to reproduce this problem but I haven't had time to work on a fix yet.

@benghopy

Can you give me an idea for fix this: I will try by myself. TKS!

@bagder
Member
bagder commented Aug 11, 2016

It is not a small and quick fix. If I had one I would already have provided it. I'm working on this.

@bagder
Member
bagder commented Aug 26, 2016

I have a patch in progress that seems to work. It would be awesome if you could try it out before I land this, as it would make me feel more safe it doesn't break things too bad.

I'm attaching it like this for now:

0001-http2-make-sure-stream-errors-don-t-needlessly-close.patch

@bagder bagder added a commit that closed this issue Aug 28, 2016
@bagder bagder http2: make sure stream errors don't needlessly close the connection
With HTTP/2 each transfer is made in an indivial logical stream over the
connection, making most previous errors that caused the connection to get
forced-closed now instead just kill the stream and not the connection.

Fixes #941
3533def
@bagder bagder closed this in 3533def Aug 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment