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. 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? Alex.