On Sun, 13 Jan 2008, Dmitry Kurochkin wrote:

> I have found time to work on curl at last. So here is another pipelining
> patch.

Great! So I take it you're also doing real-world pipelined transfers succesful
with this patch applied?

I mean, what do you consider is the quality/status of this current patch?
Would you like to see it applied as-is, since after all it is a bugfix for the
current situation?

> * server_supports_pipelining flag added to connectdata structure. It is set
> after we get first response from server and indicates if we can do
> pipelining. Before the first response comes no pipelining is done, and
> requests are stored in pend_pipe.

Two questions:

#1 - what happens when it returns a non-pipelining response? Does it
      then continue to do new connections to the server?

#2 - Shouldn't it also make sure that we're doing a request that you can
      pipeline on? I mean it needs be a GET or a HEAD for that, right?

> * SetupConnection() is not done for pipelined requests. It looks like
> only to lines from this function may be useful in pipelining:
> Curl_pgrsTime(data, TIMER_CONNECT);
> conn->now = Curl_tvnow();
> I am not sure if we really need this for http pipelining. Can
> someone please explain what are they used for?

They're used for timeouts and for time-counters. I guess the most sensible
thing to do is to perform those two lines when the actual connection is put to
use in the pipeline.

> All tests passed, build with c-ares.

Great! I only have one nit on the code:

You call the Curl_checkPendPipeline() function from multiple places in
lib/multi.c and first it seems to be you can make the function perform even
less code in case pipelining isn't actually used on the connection (you can
skip additions and assignments), but also the function is declared to return
CURLcode but the return code is never checked! It must be an oversight since
it uses Curl_addHandleToPipeline() which in fact can return an error code...

You can actually see a problem with (which I assume is) this if you run:

  ./runtests -t 530

You need to have some patience though, since the torture tests are a bit
time-consuming and in my case at least it didn't fail until iteration 1059.

(As a totally different side-note, we really need to work on decreasing the
amount of malloc()s done when the multi interface is used.)

-- 
  Commercial curl and libcurl Technical Support: http://haxx.se/curl.html