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

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? If yes
then the problem is only in the case (2).

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.

I think this is easier, at least for a short term fix, than delaying
sending body...

>>
>> If this patch is accepted, performance bug 3398 will resurface. Henrik,
>> do you think committing the patch is the right decision here even though
>> it will reopen bug 3398?
>
> Yes, but with a plan to fix it correctly.
>
> A suggested correct fix for 3398 is to make use of Expect: 100-continue.
> This delays sending the body a bit, allowing detection of broken
> connections before starting to send the body.
>
> An optimization from that is to add some buffering of request bodies to
> allow skipping the 100-continue to speed up forwarding.
>
> Regards
> Henrik
>
>> The bug title is wrong. There is a long discussion in the bug report
>> about what the bug is really about. I think a better bug title would be:
>> "persistent server connection closed before PUT".
>
> yes.
>
> Fix the bug title, reopen it with comment above and restore the check.
>
> Regatds
> Henrik
>
>
Received on Fri Aug 31 2012 - 07:58:44 MDT