Re: [PATCH] Do not reuse persistent connections for PUTs
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? If yes then the problem is only in the case (2). 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. I think this is easier, at least for a short term fix, than delaying sending body... If this patch is accepted, performance bug 3398 will resurface. Henrik, do you think committing the patch is the right decision here even though it will reopen bug 3398? Yes, but with a plan to fix it correctly. A suggested correct fix for 3398 is to make use of Expect: 100-continue. This delays sending the body a bit, allowing detection of broken connections before starting to send the body. An optimization from that is to add some buffering of request bodies to allow skipping the 100-continue to speed up forwarding. Regards Henrik The bug title is wrong. There is a long discussion in the bug report about what the bug is really about. I think a better bug title would be: persistent server connection closed before PUT. yes. Fix the bug title, reopen it with comment above and restore the check. Regatds Henrik
Re: [PATCH] cert based client authentication
ping :-) On 08/09/2012 06:55 PM, Tsantilas Christos wrote: TLS/SSL Options does not apply to the dynamically generated certificates The TLS/SSL options configured with http_port configuration parameter does not used to generate SSL_CTX context objects used to establish SSL connections. This is means that certificate based authentication, or SSL version selection and other SSL/TLS http_port options does not work for ssl-bumped connection. This patch fixes this problem. This is a Measurement Factory project
Re: [PATCH] Do not reuse persistent connections for PUTs
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.
Re: ICAP connections under heavy loads
On Thu, 2012-08-30 at 08:29 -0600, Alex Rousskov wrote:
On 08/30/2012 02:47 AM, Alexander Komyagin wrote:
What makes me wonder is that HttpStateData async job is created in
forward.cc using httpStart(fwd) function:
void
httpStart(FwdState *fwd)
{
debugs(11, 3, httpStart: \
RequestMethodStr(fwd-request-method) fwd-entry-url()
\ );
AsyncJob::Start(new HttpStateData(fwd));
}
And I do not see any way for the request forwarder to control that job!
Yes, the relationship among client-side, FwdState, and server-sde is
weird and wrong on many counts. With some minor exceptions, there is no
direct control of the server-side from the client-side and vice versa.
Client-side-driven termination of server-side jobs is usually triggered
via StoreEntry aborts, but HttpState does not receive StoreEntry abort
notifications (a design bug) -- it checks for them when something else
happens. This is something we should improve.
For example, let's assume that our ICAP service successfully checked the
request but we are unable to connect to ICAP in order to check the reply
- just do NOP in Xaction openConnection() method if it was initiated by
HttpStateData.
This way, after client timeout occurs, Squid properly detects it but
nothing is done to stop HttpStateData job.
Which may be the desired outcome in some cases (e.g.,quick_abort caching
and I want to see everything ICAP service).
In real world Xaction will use noteCommTimeout to handle this case and
destruct the job; but still it looks strange for me that Squid doesn't
abort HttpStateData job in case the client is gone.
It is complicated because Squid has to balance the needs of each single
client with the needs of ICAP services and other clients (including
future ones if caching is enabled).
and meaninglessly switches icap status in minutes after the test.
Why do you describe a down status of an overloaded ICAP service as
meaningless? The status sounds appropriate to me! Again, I do not know
much about httperf internals, but in a real world (or a corresponding
Polygraph test), the ICAP service may be always overwhelmed if there is
too much traffic so a down state would be warranted in many such cases
(Squid, of course, cannot predict whether the service overload is
temporary).
I think it's better to detect ICAP down state as earlier as we can,
and certainly not after minutes after the load peak. The thing is that
in that 6 minutes ICAP service becomes responsive again, and then Squid
eventually catches dozens of exceptions and turns ICAP down. And that's
is a mistake, because ICAP is OK now!
Some of the up/down decision logic is configurable, and if you have
suggestions on how to improve it, let's discuss them! The trick here is
to maximize the number of common ICAP service failures that the
algorithm can handle in a reasonable fashion, without adding 10 more
configuration options.
Correct me if I'm wrong, but a proper way to fix the issue would be to
deny Xaction activity until the connection is fully established, since
the problem is not in the read/write operations but in the connection
opening.
What do you mean by deny Xaction activity? I do not think current ICAP
transactions do much while they wait for the connection to be
established. They just accumulate virgin data and wait. What should they
do instead?
This way Xaction object will detect connection fail after connection
timeout as it's supposed to be.
If ICAP transactions do not detect connection timeout now, it is a bug
that we should fix.
Alex, I figured it out, finally! The bug was in comm_connect_addr()
function (I suppose it is kernel-dependent though).
Consider following call trace:
1) Xaction starts ConnOpener in order to create new connection to ICAP
2) ConnOpener calls comm_connect_addr()
3) comm_connect_addr() initiates connection through connect() and
returns COMM_INPROGRESS
...since our ICAP service is too busy connection will eventually timeout
(connectTimeout_), so...
4) Comm::ConnOpener::timeout() is called
5) Comm::ConnOpener::timeout calls connect()
6) connect() calls comm_connect_addr()
7) comm_connect_addr() calls getsockopt(SOL_SOCKET, SO_ERROR) to check
current connection status
8) getsockopt(..) returns errno 0 --- BUG
9) comm_connect_addr() returns COMM_OK so ConnOpener would think that
connection was successfully established and pass the connection back to
Xaction object, then we would have noteCommRead and noteCommWrote
exceptions and so on...
According to `man connect`, when connect() returns errno EINPROGRESS:
EINPROGRESS
The socket is nonblocking and the connection cannot be completed
immediately. It is possible to select(2) or poll(2) for
completion by selecting the socket for writing. After select(2)
indicates writability, use getsockopt(2) to read the SO_ERROR
option at level
Re: ICAP connections under heavy loads
On 08/31/2012 09:01 AM, Alexander Komyagin wrote:
Alex, I figured it out, finally! The bug was in comm_connect_addr()
function (I suppose it is kernel-dependent though).
Consider following call trace:
1) Xaction starts ConnOpener in order to create new connection to ICAP
2) ConnOpener calls comm_connect_addr()
3) comm_connect_addr() initiates connection through connect() and
returns COMM_INPROGRESS
...since our ICAP service is too busy connection will eventually timeout
(connectTimeout_), so...
4) Comm::ConnOpener::timeout() is called
5) Comm::ConnOpener::timeout calls connect()
6) connect() calls comm_connect_addr()
7) comm_connect_addr() calls getsockopt(SOL_SOCKET, SO_ERROR) to check
current connection status
8) getsockopt(..) returns errno 0 --- BUG
Actually, the bug is in step #5. If we timed out, we should not try to
connect again.
Step #8 seems correct -- from the OS point of view, internal Squid
timeout is not an error. If you fix step #5, step #8 will not happen though.
9) comm_connect_addr() returns COMM_OK so ConnOpener would think that
connection was successfully established and pass the connection back to
Xaction object, then we would have noteCommRead and noteCommWrote
exceptions and so on...
According to `man connect`, when connect() returns errno EINPROGRESS:
EINPROGRESS
The socket is nonblocking and the connection cannot be completed
immediately. It is possible to select(2) or poll(2) for
completion by selecting the socket for writing. After select(2)
indicates writability, use getsockopt(2) to read the SO_ERROR
option at level SOL_SOCKET to determine whether connect()
completed successfully (SO_ERROR is zero) or unsuccessfully
(SO_ERROR is one of the usual error codes listed here,
explaining the reason for the failure).
Actually ConnOpener uses SetSelect(..) in order to wait before calling
comm_connect_addr() (and connect() ) again, but timeout in fact breaks
the rule calling getsockopt() right after connect().
The above man page excerpt says to call getsockopt() after successful
select() notification. I think that is what we do now.
I do not think we are or should be calling connect() twice -- the OS
knows that we want to connect and does not need reminders. Note the
(!F-flags.called_connect) guard in comm.cc.
If you are looking for connect-related bugs, I recommend fixing step #5
and also examining this hack:
x = connect(sock, AI-ai_addr, AI-ai_addrlen);
// XXX: ICAP code refuses callbacks during a pending comm_ call
// Async calls development will fix this.
if (x == 0) {
x = -1;
errno = EINPROGRESS;
}
I hope the above XXX code is no longer needed (and removing it will save
a lot of CPU cycles for everybody, not just ICAP!), but this needs to be
double checked using bzr logs to restore the exact reason behind that
hack and by examining the current ICAP code.
In my environment we use uClibc and rsbac kernel, but I suppose that the
problem may persist in other environment too.
I've attached two small patches. One - to avoid connection loss in case
client aborted the request before connection to the ICAP was established
(remember that BUG: Orphaned... stuff I asked you before?).
Another one is to actually fix the ICAP connection timeout (not only
ICAP actually) problem. Comments will be appreciated.
* For the conn-open-timeout patch:
Overall, I think this patch is fighting the side-effects of other bugs.
Most importantly, the timeout handler should abort the ConnOpener job on
the spot rather than go through one more select() try. I will comment on
specific changes below, just in case, but please keep this overall
problem in mind.
case COMM_INPROGRESS:
// check for timeout FIRST.
-if (squid_curtime - connectStart_ connectTimeout_) {
+if (squid_curtime - connectStart_ = connectTimeout_) {
debugs(5, 5, HERE conn_ : * - ERR took too long
already.);
Please revert this change. In theory, we can be called back again before
(squid_curtime - connectStart_) changes. Not a big deal, of course, (the
equality should be a very rare event) but the old code was more accurate
from logic/math point of view.
Also, if the debug message were to print by how much we were late,
printing zero would look strange.
If there is some hidden side-effect or dependency here that I missed,
please let me know (but note that I think this code must not be called
from the timeout handler -- as discussed above, the timeout handler
should abort instead of double checking whether there is a timeout).
++failRetries_;
// check for timeout FIRST.
-if (squid_curtime - connectStart_ connectTimeout_) {
+if (squid_curtime - connectStart_ = connectTimeout_) {
debugs(5, 5, HERE conn_ : * - ERR took too long to
receive
Re: [PATCH] Do not reuse persistent connections for PUTs
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.
