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

On 08/29/2012 02:32 PM, Henrik Nordström wrote:
> ons 2012-08-29 klockan 22:20 +0200 skrev Henrik Nordström:
>> ons 2012-08-29 klockan 12:42 -0600 skrev Alex Rousskov:
>>> I saw bogus ERR_ZERO_SIZE_OBJECT responses while testing Squid v3.1,
>>> but the same problem ought to be present in v3.2 as well.
>>>
>>> A compliant proxy may retry PUTs, but Squid lacks the [rather
>>> complicated] code required to protect the PUT request body from being
>>> nibbled during the first try, when pconn races are possible.
>>
>> +1 on the patch from me.
>
> It's a reincarnation of Bug #569, kind of. The check for a request body
> have "always" been there (well since that bug was fixed), but was
> deleted in revision 11589 for some reason.

Indeed! I think Christos deleted the check to address the complaint that
Squid was closing a previously established pconn before sending a new
POST or PUT.

What nobody (including me) realized is that Squid is not yet ready to
retry some PUTs that fail due to a pconn race. There is code to check
that we do not retry a PUT when we cannot retry it, but there is no code
to preserve PUT so that it can be retried if needed. That missing code
would be quite tricky to write correctly.

Today, we have a choice:

  A. Fail some PUTs. Close fewer pconns (current code).
  B. Handle all PUTs. Open more connections (patched code).

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?

Regardless of the A/B decision today, it would be nice to add code to
protect [small] PUT contents if necessary, so that we can do (B) without
sacrificing performance in common cases. That requires a lot more work
as I already mentioned above.

> Log message on the delete says
>
> Bug 3398: persistent server connection closed after PUT/DELETE

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".

Thank you,

Alex.
Received on Wed Aug 29 2012 - 21:33:07 MDT