curl-library
Re: [PATCH 2/7] pipelining: Fix connection handling under timeouts.
Date: Sun, 16 Nov 2014 19:26:36 +0100 (CET)
On Fri, 14 Nov 2014, Carlo Wood wrote:
> + if(data->mstate != CURLM_STATE_DONE)
> + multistate(data, CURLM_STATE_DONE);
> + else
> + /* if already in DONE, go COMPLETED */
> + multistate(data, CURLM_STATE_COMPLETED);
>
> as stated before in a reply from me, this is incorrect; when data->mstate ==
> CURLM_STATE_DONE here, then ONLY multistate(data, CURLM_STATE_DONE) was
> called so far, which only set data->mstate to CURLM_STATE_DONE and nothing
> else was done yet: we certainly should still *run* that state!
Your logic make total sense here, but it itself highlights to me why this is
the wrong path for us to take. We shouldn't continue operating in a state when
we already triggered a timeout. We then need to instead leave the state and go
to completed. You can for example see how there's a flow in the DONE state
that goes back to INIT which is just wrong the we already timed out. And the
are protocols (like FTP) that may spend a long time in the Curl_done() call
which also is wrong as we have already concluded that this handle has timed
out. It makes the timeout lose precision.
The disconnect from the pipeline and its connection should then be done in the
check below the switch as it was always intended. Something like the attached
patch.
> - if((conn->handler->protocol & PROTO_FAMILY_HTTP) &&
> + if(!conn->bits.close &&
> + (conn->handler->protocol & PROTO_FAMILY_HTTP) &&
>
> That disables pipelining completely for the site in question no? It seems
> that this is way too soon and too strong.
Ah yes indeed, it checks the 'needle' one for this too which isn't what I
intended... that's not a good check then. I believe a variation of your
original check is better. I'm trying with such a one in my patch suggestion
here.
This patch is still for discussion purposes, it is not complete and it causes
two test case failures (1208 and 1513) for me.
Thoughts?
-- / daniel.haxx.se
-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html
- TEXT/x-diff attachment: 0001-multi-when-leaving-for-timeout-close-accordingly.patch