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.
