Re: [PATCH] Consumption of POST body

On 18.01.13 18:37, Alex Rousskov wrote:

Further to this...

> * Use expectNoForwarding() instead of adding forwardToBitbucket().
>
> * In ConnStateData::noteBodyConsumerAborted() do not stop reading if
> flags.readMore is set. Just keep auto-consuming. This may prevent
> authenticating connections closures even though expectNoForwarding() is
> used. Needs to be tested: Will we correctly stop and switch to the new
> request when the body finally ends?

I have changed to using expectNoForwarding() and removed the
stopReceiving() call from noteBodyConsumerAborted() and this appears to
work correctly:

1. Making a request which results in an error, with "Connection: close"
produces a response to the client, and then once the request body has
been fully received, Squid closes the connection.

2. Making a request which results in an error, with "Connection:
keep-alive" produces a response to the client, and then once the request
body has been fully received Squid reads the next request.

3. Making a request which does *NOT* result in an error, with
"Connection: keep-alive" works as expected and then waits for the next
request.

4. Making a request which does *NOT* result in an error, with
"Connection: close" works as expected and then closes the connection.

5. Making a request which results in an ICAP error (I shut down a
non-bypassable ICAP service), with "Connection: keep-alive" produces a
500 error response and then waits for the next request.

6. Making a request which results in an ICAP error, with "Connection:
close" produces a 500 error response and then Squid closes the connection.

I don't really understand the purpose of the readMore flag, can you
explain it?

> * Discuss and possibly remove readNextRequest setting from
> ClientHttpRequest::handleAdaptationFailure(), to remove inconsistency
> discussed in #2 above.

As mentioned above, I don't understand what readMore is actually for.
However, as I mentioned in my previous email, the only time I can see us
wanting to blindly close the connection for adaptation failures is when
we've already started sending a response to the client and the
adaptation service blows up in the middle of the response body. It
doesn't _look_ like handleAdaptationFailure() handles that event though,
unless I'm mistaken?

> * Discuss whether we are OK with clients tying up Squid after an
> [authentication] error. Perhaps we need another timeout there, perhaps
> there are already mechanisms to address that, or perhaps we do not
> really care.

There are 4 possible ways I can see of a client tieing up Squid:

1. The client connects but doesn't send anything.
2. The client connects, makes a keep-alive request that results in an
error, then doesn't send any new request.
3. The client makes a request that generates an error, but doesn't send
the complete request body.
4. The client makes a request that generates an error, uses chunked
encoding and sends an infinitely long body.

(1) and (2) are basically the same thing. I've tested this and Squid
times out the connections and closes them after a few minutes.

(3) similarly gets timed out after a while.

I haven't tested (4), but I have no reason to believe there's anything
that currently stops it from happening. I'm not sure whether or not we
should do something to prevent it - that's one for discussion.

-- 
  - Steve Hill
    Technical Director
    Opendium Limited     http://www.opendium.com
Direct contacts:
    Instant messager: xmpp:steve_at_opendium.com
    Email:            steve_at_opendium.com
    Phone:            sip:steve_at_opendium.com
Sales / enquiries contacts:
    Email:            sales_at_opendium.com
    Phone:            +44-844-9791439 / sip:sales_at_opendium.com
Support contacts:
    Email:            support_at_opendium.com
    Phone:            +44-844-4844916 / sip:support_at_opendium.com