On 15/01/2013 9:12 a.m., Rainer Weikusat wrote:
The attached patch moves the body of the timeout handling code from the ConnOpener::connect method to the ConnOpener::timeout method instead of invoking ::connect from ::timeout in order to make squid enforced connect timeouts work (that is, abort the connection) instead of wrongly making squid believe that the connect succeeded because the getsockopt call done in comm_connect_addr won't return an error for this case. This also means that there's now one connect timeout check done in checkTimeouts instead of two whose results will usually differ. An internal method named 'stillConnecting' is introduced which contains the check for an 'abort done by the parent' which is presently in ConnOpener::connect,// our parent Jobs signal abort by cancelling their callbacks. if (callback_ == NULL || callback_->canceled()) return; This method is called from both ::connect and ::timeout to determine if the state of the connect request is still undecided by the time either of both methods executes. This guards against the case that both ::connect and ::timeout are scheduled before one of them actually runs and detects when the 'other side' of a full connection has already gone away or is in the process of going away (and hence, the already existing 'result state' should be kept).
The stillConnecting() mechanism should not be needed at all. The doneAll() method tests should be what is used to determine whether the Job is still running or not.
That sad, it should only be necessary to test in the connect() handler. The timeout handler call is held in calls_.timeout_ and should be cancel()'ed during the doneConnecting() process after any form of completion.
Amos
