On 19/11/2011 7:01 a.m., Alex Rousskov wrote:
On 11/17/2011 08:15 PM, Amos Jeffries wrote:
On 18/11/2011 11:51 a.m., Alex Rousskov wrote:
Closed bug 3190, open bug 3420, open bug 3417, and several
unreported problems deal with stuck requests, early responses, aborted
requests/responses and combination of those. The attached patch attempts
to solve many of these mushrooming problems, but it is a significant
change that may introduce other bugs. Please review.
The patch is against v3.1 because most sufferers are running that
version. I will port changes to trunk if approved. Technical details are
copied below from the patch preamble.
Fine. The change itself appears very simple. How much testing has it
been through?
None beyond very limited lab tests to verify that it solves at least
some of the bugs reported and does not crash under basic requests. At
this time, I do not consider this patch well tested. (Nor do I consider
the change simple, but that is not important :-).
TODO: This patch focuses on various error cases. We might still have
problems when there is an early HTTP response and no errors of any kind.
I marked the corresponding old code with an XXX.
I'm not sure what you are thinking here. Early response with a
keep-alive should not be hitting the close / stopSending() code path
unless its an error.
True. The problem starts in ClientSocketContext::keepaliveNextRequest
part of the code which handles the "no error" cases. See the new XXX
added by the patch there.
And early response with a Connection:close leaving
the clients side (because server-side pconn is irrelevant) can be closed
fully just fine after the write.
The problem may exist for early responses on persistent connections,
with no errors of any kind.
Do we need any clean and purge logics to empty the pipeline without
errors being set on that early close case?
It is not an "early close" case. Just an "early response" case.
See ClientSocketContext::keepaliveNextRequest(). Assume no errors of any
kind but also assume that we are still reading the request body (i.e.,
that we are dealing with an early response). Observe that the call to
clientParseRequest() will fail to read the next request, as it should.
However, I suspect that we may then call readNextRequest() which might
actually start reading the next request using the current request body.
I may very well be wrong. Somebody should test whether it could happen
like that, using carefully crafted transactions. It will be difficult to
bypass all the protections we already have, but perhaps not impossible.
Conceptually, it is clearly wrong to call clientParseRequest() or
readNextRequest() when we are still not done with the current request.
Those methods are named and written to deal with "next" requests.
Whether current protections in those methods are sufficient to prevent
request smuggling is a question asked by the XXX I added.
A probably much simpler alternative is to add some code to
ClientSocketContext::keepaliveNextRequest() to make sure those two "next
request" methods are not called if we are not done with the current
request AND also check that there is some other code to call those
methods when we are done with the current request.
Ah, thanks. understood now.
In that scenario I think you are right on that last paragraph. The "some
other code" IMO should be keepAliveNextRequest() itself which I think
should be called when (a) the response is finished, and (b) the request
is finished. Checking that in case (a) the request is finished arriving,
and case (b) either pipeline is active or both request and response are
finished; before kicking off the parse operations on next request. If
one of the two is not finished just exiting, without scheduling anything
while waiting for the call which will happen when that other has finished.
In that line of design we could probably drop the "keepalive" part of
the name. Just calling it nextRequest() and document that it is
responsible for whatever happens to the client connection on successful
request/response completion.
Its assumption that there are no errors seems okay. Since the errors
take other failure code paths. Just this weird excess of request data
being pushed in legitimately with a C-L header or chunking.
The edge case is likely to be
ClientSocketContext::keepaliveNextRequest() where pinning closes the
conn. That is one error case and could potentially lead to problems. I
suspect that it should be doing stopSending("pinned server gone") before
it does comm_close(). Do you agree?
I am afraid no. There is no point in calling stopSending (or
stopReceiving) before calling comm_close() because comm_close() will
kill the connection and we would have to stop sending and receiving.
I was thinking more along the lines of passing the reason for closure.
Its not causing any bad behaviour that I can see.
Amos
