On 08/31/2012 05:31 PM, Alex Rousskov wrote: > On 08/31/2012 01:58 AM, Tsantilas Christos wrote: >> On 08/30/2012 06:11 AM, Henrik Nordström wrote: >>> ons 2012-08-29 klockan 15:32 -0600 skrev Alex Rousskov: >>> >>>> Today, we have a choice: >>>> >>>> A. Fail some PUTs. Close fewer pconns (current code). >>>> B. Handle all PUTs. Open more connections (patched code). >> >> The FwdState::checkRetriable() method is used in the following two cases: >> >> 1) To decide if it can reuse a healthy persistent connection. Inside >> FwdState::connectStart, we are getting a persistant connection and even >> if it is healthy, if we want to send eg a PUT request we are closing the >> persistent connection and we are opening a new one. >> >> 2) To decide if a connection can be retried (inside FwdState::checkRetry()) >> >> From what I can understand there is not any reason to not reuse a >> healthy persistent connection, for PUT requests. Am I correct? > > I am afraid you are not correct. There is a reason. Today, the code may > destroy PUT content before we can detect a pconn race. If we reuse a > pconn for PUT, we may later discover a pconn race but would be unable to > retry the failed PUT because the PUT content would have been already > nibbled by then.
I see... > > In other words (and simplifying a bit), the current "not nibbled" > condition in checkRetriable() should be replaced with "not nibbled and > will not be nibbled" condition. Since we cannot guarantee the "will not" > part, we have to exclude all requests carrying body from being sent on > reused pconns. > > I can reproduce this bug in the lab so it is not just a theory: > HttpStateData consumes PUT request body and only then gets a zero-length > read from the server, resulting in ERR_ZERO_SIZE_OBJECT returned to the > innocent client. > > > >> In the case (2) we are using the "if (request->bodyNibbled())" to >> examine if any body data consumed and probably sent to the server. So we >> should not have any problem at all. >> However in the case the HttpRequest::bodyNibbled() is not enough we can >> add the check "if (request->body_pipe != NULL)" inside >> FwdState::checkRetry() method after calling checkRetriable. > > The current bodyNibbled() check is enough to avoid sending a nibbled > request to the server. Unfortunately, that means we will not retry some > failed PUTs, and we must retry them. > > In other words, after (the pconn race happened and we nibbled) it is too > late to fix the situation by retrying a PUT. Thus, we have to prevent > either pconn races or nibbling. We can easily prevent races (the > proposed patch does that). Eventually, we will prevent nibbling (to get > a performance boost). > > > Does this clarify? Yep. So looks that a good solution should be (similar to) the solution proposed by Henrik... > > Alex. > >