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

On 19/01/2013 12:32 p.m., Rainer Weikusat wrote:
> Alex Rousskov <rousskov_at_measurement-factory.com> writes:
>> On 01/17/2013 02:53 PM, Rainer Weikusat wrote:
> [...]
>
>>> If the write handler was scheduled
>>> by the time timout runs, this happened because 'something definite' is
>>> known about the state of the connection attempt: Either, it suceeded
>>> or the OS returned an error. Throwing this 'real information' away in
>>> order to save a line of code seems not right to me even if it is a
>>> fringe case of a fringe case.
>> As I said, one would not be just saving lines of code. One would be most
>> likely introducing bugs (that is exactly what happened in the past with
>> this code). We have at most two "real information" pieces: a timeout and
>> I/O status. The first to get to ConnOpener wins.
> Since F->write_handler will be cleared prior to the write handler
> being scheduled, the ::timeout code could exploit this property to
> determine reliably if an I/O status is available. The question would
> be 'is this useful enough to warrant otherwise needlessly digging
> around in the guts of another module'?

This is the debate I lost with Alex. We are talking sub-millisecond race
resolution on something which has already passed >1 minute of wait time
at the client end. There is a high chance that we have also lost the
client disconnection race some whole seconds ago, or that processing
this timeout() is holding up an I/O select() Call which will change that
write handlers status.

I still think it would be worth checking if it were proven that calling
connect() again would be reliable. But the benefit of doing so in the
current code is outweighed by the complexity needed for reliability.

> A 'nicer' solution would be to
> add an interface to comm which could look like this:
>
> void *comm_kill_read_handler_if(int fd);
> void *comm_kill_write_handler_if(int fd);
>
> which would do the necessary fd_table manipulations to kill a read or
> write handler and return the read_data/ write_data pointer if a
> handler was actually registered. Considering 'This is a hack,
> polishing it now is not a good idea.'
> (<50F8D7D3.3010903_at_treenet.co.nz>), I think you're right and not
> checking for this is the better choice.

Yes, we an come back and look at this after the SetSelect() code is
upgraded to AsyncCall awareness.

Amos
Received on Sat Jan 19 2013 - 04:10:59 MST