On 19/01/2013 12:32 p.m., Rainer Weikusat wrote:
Alex Rousskov <[email protected]> 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.'
(<[email protected]>), 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
