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

On 17/01/2013 6:53 p.m., Alex Rousskov wrote:
> On 01/16/2013 09:58 PM, Amos Jeffries wrote:
>
>> Whet I was trying to point out is that we have three race conditions
>> between timeout() and connect() Calls...
>>
>> * connect() happens first and runs re-schedules while timeout() is queued.
>> - this is the case we are now ignoring. I'm fine with that.
> Yes (a failed connect wins and retries).
>
>> * connect() happens first and runs doneConnection() while timeout() is
>> queued.
> Yes (a failed connect wins and ends ConnOpener job).
>
>> - in this case doneConnecting() will cancel the timeout() queued Call.
> There is no need to cancel any pending calls to ConnOpener. The async
> call will simply not be delivered to the destroyed ConnOpener job. What
> we do need is to cleanup anything we have allocated (that cleanup should
> not be in the timeout callback or, at least, that cleanup should not be
> exclusive to the timeout callback because the callback may never be called).

It is currently done by doneConnecting(). Which is the correct way to
exit ConnOpener with success/error status.

>
>> * timeout() happens first and runs doneConnecting() while connect() is
>> queued.
> Yes (timeout wins and ends ConnOpener job).
>
>
>> - we are still connecting because the connect() Call has not run.
> Yes, that is possible. We should tell Comm to stop connecting (if Comm
> still does), to avoid wasting resources, but everything ought to work
> correctly even if we do not tell Comm.
>
>
>> - we have no handle to the connect() AsyncCall stored so we can't
>> cancel it directly (bug?).
> We do not need to cancel it (just like with the timeout call discussed
> above). The async call will not be delivered to the destroyed ConnOpener
> job).

Okay. If the Async code prevents the ConnOpener::connect() function
being run, then the original if() which stillConnecting() replaces there
is now obsolete.

Amos
Received on Thu Jan 17 2013 - 07:02:49 MST