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

On 08/31/2012 05:31 PM, Alex Rousskov wrote:
> 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.

I see...

>
> 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?

Yep.
So looks that a good solution should be (similar to) the solution
proposed by Henrik...

>
> Alex.
>
>
Received on Fri Aug 31 2012 - 18:44:45 MDT