2008/1/13, Daniel Stenberg <daniel_at_haxx.se>:
> 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 ran a small app, but no real-world usage yet.

> 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?
I think it needs more testing. Probably it can be applied since it
affects only pipelining. But that is not for me to decide.

> > * 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?
It should work the following way. New requests are added to pend_pipe,
but they are not moved to send pipe is server_supports_pipelining. So
there will be no new connections (for requests that can be pipelined).
When current connection is closed, signalPipeClose() restarts requests
in pend_pipe.

> #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?
ConnectionExists() checks that request is GET or HEAD already. So PUT
request will create a new connection as usual.

Note, that both #1 and #2 are not actually tested...

> > * 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.
Ok. I will put this lines into Curl_checkPendPipeline().

> > 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...
Yes. This should be fixed. I want to get rid of malloc there: new
function to move elements from one list to another without any
malloc/free.

> 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.
Wow. Did not know of this feature in testsuite. It is very impressive,
gdb option helped me a lot already!

But I could not reproduce this:

test 530...[HTTP GET using pipelining]
 1040 allocations to make fail
torture OK
TESTDONE: 1 tests out of 1 reported OK: 100%
TESTDONE: 1 tests were considered during 178 seconds.

No failure... Any ideas why?

> (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.)

The function to move element from one list to another I mentioned
above. It can be used to remove malloc/free when moving request from
send to recv pipe.

Regards,
  Dmitry

>
> --
> Commercial curl and libcurl Technical Support: http://haxx.se/curl.html
>
Received on 2008-01-13