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

My apologies for the botched threading but I picked this message out
of the MARC WWW archive since I'm not subscribed to squid-dev and
don't plan to subscribe because minus problems encountered when using
squid 3.3 in a particular 'production' setup, I don't plan to be
involved with squid development and wouldn't have time to read the
list traffic, anyway (thanks to general 'work overload', not
necessarily because I wouldn't be interested ...). It would be very
kind of anybody answering to this could Cc: me because of this.

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

These two lines of code are presently at the top of
ConnOpener::connect. If it should really be ConnOpener::doneAll there,
than, the new method is obviously redundant, but that's something I
don't know (since I didn't wrote the code containing the two slightly
different 'still something to do' checks and if in doubt, I'd rather
assume that there was a reason for that).

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

I think this isn't necessarily true for two reasons:

- while testing in order to track down the original HTTPS timeout
issue, I determined that the same problem doesn't happen for HTTP even
despite (some parts of) the same code are executed. As far as I could
determine, the difference is that destruction of the FwdState was
already initiated because of an event on the client-side of the
'virtual end-to-end connection': For an HTTP timeout, at least some
times, ::timeout gets called and then ::connect and then ::connect
returns because of the callback check (I've checked this with the help
of some ad hoc debugging code). Consequently, when ::timeout doesn't
call ::connect, the same situation can still occur and since
'everything works' for this case, not doing any timeout processing in
case the callback check had terminated ::connect early seemed the
right thing to do for me

- both ::connect and ::timeout aren't executed directly when the
respective events occur but executed from the main loop using a
'delayed, scheduled call' mechanism. At least in theory, it seems
possible that both are scheduled before either executes. One of the
messages in the 'ICAP connections under heavy load' thread explicitly
mentioned this possibility, too. And the timeout code shouldn't generated
any diagnostics about connections which timed out when they actually
didn't.
Received on Tue Jan 15 2013 - 14:39:36 MST