Re: [PATCH] Move timeout handling from ConnOpener::connect to ConnOpener::timeout

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
Received on Mon Jan 14 2013 - 22:15:33 MST