Re: [PATCH] Consumption of POST body

On 01/18/2013 08:14 AM, Steve Hill wrote:

> The attached patch enables autoconsumption of the BodyPipe if access
> isn't allowed in order to discard the request body rather than letting
> it fill the buffers.

Please note that the patch enables auto-consumption for all callout
errors, not just access denials.

Also, the trunk code is even more complex in this area. I believe your
patch, when carefully ported to trunk, may enable auto-consumption for
all callout errors *that have readNextRequest set*. That makes sense to
me: if we are going to read the next request, we should consume the
current one without closing the client connection so we should not call
expectNoForwarding() that may have that side effect.

The overall intent sounds correct to me. Even if we do not want to keep
the connection open after responding with an error, it may be impossible
to deliver the error message to a half-broken client that does not read
while writing to us.

However, this brings us back to trunk r11920 and bug 3420. While that
fix was triggered by an assertion, it is clear from the code comments
and the commit message that we were worried about a client tying Squid
client connection "forever" (with or without sending data). Your changes
will re-enable that behavior in some cases, I guess. More on that below.

> I'm not entirely sure this is the best fix - calling
> expectNoForwarding() would seem to be neater, but this also causes the
> client-side socket to close, which would break authentication mechanisms
> that require the connection to be kept alive. Hopefully someone with a
> better understanding of the Squid internals can review the patch and
> tell me if it is the right approach?

Thank you very much for treading carefully and disclosing potential
problems!

Indeed, having two methods with approximately the same purpose and a
hidden critical distinction is asking for trouble. A lot of existing
expectNoForwarding-path code comments describe the situation that
matches yours. Moreover, as far as I can see, we may call the old
expectNoForwarding() first and then call your forwardToBitbucket()
later, all for the same error!

As far as I can tell, we have several cases to take care of (or ignore)
here:

1. Authentication: Must read next request on the same connection so must
have readNextRequest set (if not, this is probably a bug). Have to live
with the possibility of the client sending forever.

2. Adaptation errors: The code may have been written to abort the client
connection, but it also sets readMore or readNextRequest. This sounds
inconsistent. We should review this, especially since this is the only
place where we call expectNoForwarding() today!

3. Other errors: These may or may not have readNextRequest set. Let's
assume that they set (or not set) that flag correctly.

Suggestions:

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

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

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

What do you think?

Alex.
Received on Fri Jan 18 2013 - 18:37:27 MST