On Thu, 6 Nov 2014, Carlo Wood wrote:

> If a request on a pipe times out, it breaks that pipe: the
> connection needs to be closed. However, when doing http pipelining
> we really don't want to delete the bundle and forget that this
> server can do pipelining because that results in a separate connection
> for every handle in the pipeline that didn't timeout yet, plus one
> for every handle that is added until we receive new headers from the
> server again, potentially a lot.

I have a somewhat pedantic disagreement with the bundle
structure as currently defined. It really feels like the
'enabled for pipelining' state should be per-connection and
not per-bundle/host-hash. The hash does not uniquely
identify a service instance, a process or even a host. In
practice, this is not the biggest hurdle pipelining faces.
So I'll just shrug it all off...

<snip>

> ---
> lib/multi.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/lib/multi.c b/lib/multi.c
> index add396d..fde54fa 100644
> --- a/lib/multi.c
> +++ b/lib/multi.c
> @@ -1646,15 +1646,31 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
>
> if(data->easy_conn) {
> CURLcode res;
> + struct connectbundle *bundle;
> + bool keep_bundle;
>
> /* Remove ourselves from the receive pipeline, if we are there. */
> Curl_removeHandleFromPipeline(data, data->easy_conn->recv_pipe);
> /* Check if we can move pending requests to send pipe */
> Curl_multi_process_pending_handles(multi);
>
> + /* If we're doing pipelining and this connection timed out then we do
> + not want Curl_done -> Curl_disconnect -> Curl_conncache_remove_conn
> + to remove the bundle: we need to remember that this server is
> + capable of pipelining. */
> + bundle = data->easy_conn->bundle;
> + keep_bundle = (bundle->server_supports_pipelining &&
> + data->result == CURLE_OPERATION_TIMEDOUT);
> + if(keep_bundle)
> + ++bundle->num_connections;
> +
> /* post-transfer command */
> res = Curl_done(&data->easy_conn, data->result, FALSE);
>
> + /* Restore num_connections to its original value. */
> + if(keep_bundle)
> + --bundle->num_connections;
> +
> /* allow a previously set error code take precedence */
> if(!data->result)
> data->result = res;

Okay, it looks like you are getting away with this. But the
evil is this: You've changed the assumptions around the
bundle behavior. Before existence of a bundle meant at
least one connection in the bundle. Now that's not true.
But you've changed this in code far, far removed from
bundles.[hc] or conncache.[ch]. It's been left undocumented
and untested in the code that actually owns the behavior.

--
Monty Brandenberg, Software Engineer                               MCB, Inc.
mcbinc_at_panix.com                                             P.O. Box 425292
mcbinc_at_pobox.com                                   Cambridge, MA  02142-0006
617.864.6907
-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette:  http://curl.haxx.se/mail/etiquette.html