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.
