Re: [PATCH] Do not reuse persistent connections for PUTs

On 08/31/2012 01:58 AM, Tsantilas Christos wrote:
> On 08/30/2012 06:11 AM, Henrik Nordström wrote:
>> ons 2012-08-29 klockan 15:32 -0600 skrev Alex Rousskov:
>>
>>> Today, we have a choice:
>>>
>>> A. Fail some PUTs. Close fewer pconns (current code).
>>> B. Handle all PUTs. Open more connections (patched code).
>
> The FwdState::checkRetriable() method is used in the following two cases:
>
> 1) To decide if it can reuse a healthy persistent connection. Inside
> FwdState::connectStart, we are getting a persistant connection and even
> if it is healthy, if we want to send eg a PUT request we are closing the
> persistent connection and we are opening a new one.
>
> 2) To decide if a connection can be retried (inside FwdState::checkRetry())
>
> From what I can understand there is not any reason to not reuse a
> healthy persistent connection, for PUT requests. Am I correct?

I am afraid you are not correct. There is a reason. Today, the code may
destroy PUT content before we can detect a pconn race. If we reuse a
pconn for PUT, we may later discover a pconn race but would be unable to
retry the failed PUT because the PUT content would have been already
nibbled by then.

In other words (and simplifying a bit), the current "not nibbled"
condition in checkRetriable() should be replaced with "not nibbled and
will not be nibbled" condition. Since we cannot guarantee the "will not"
part, we have to exclude all requests carrying body from being sent on
reused pconns.

I can reproduce this bug in the lab so it is not just a theory:
HttpStateData consumes PUT request body and only then gets a zero-length
read from the server, resulting in ERR_ZERO_SIZE_OBJECT returned to the
innocent client.

> In the case (2) we are using the "if (request->bodyNibbled())" to
> examine if any body data consumed and probably sent to the server. So we
> should not have any problem at all.
> However in the case the HttpRequest::bodyNibbled() is not enough we can
> add the check "if (request->body_pipe != NULL)" inside
> FwdState::checkRetry() method after calling checkRetriable.

The current bodyNibbled() check is enough to avoid sending a nibbled
request to the server. Unfortunately, that means we will not retry some
failed PUTs, and we must retry them.

In other words, after (the pconn race happened and we nibbled) it is too
late to fix the situation by retrying a PUT. Thus, we have to prevent
either pconn races or nibbling. We can easily prevent races (the
proposed patch does that). Eventually, we will prevent nibbling (to get
a performance boost).

Does this clarify?

Alex.
Received on Fri Aug 31 2012 - 14:31:53 MDT