On 18/01/2013 10:53 a.m., Rainer Weikusat wrote:
Alex Rousskov <[email protected]> writes:
On 01/17/2013 10:56 AM, Rainer Weikusat wrote:
Amos, do you remember what "partially open FD" in the following comment
meant, exactly? Why cannot we just call comm_close() to cleanup?
// it never reached fully open, so cleanup the FD handlers
// Note that comm_close() sequence does not happen for partially open FD
In your latest patch, please move fd cleanup from timeout() and from
doneConnecting() into a dedicated method and call that method from
swanSong() if temporaryFd_ is still set there. Do not call it from
doneConnecting() or anywhere else. Remember that a job may end for
reasons other than some timeout or I/O. swanSong() is the cleanup method.
Please do not forget to set calls.earlyAbort to NULL after canceling it
in the fd cleanup method.
What precisely constitutes 'fd_cleanup'? The doneConnecting code will
set calls_.early_abort to NULL and the leading comment states
/**
* Connection attempt are completed. One way or the other.
* Pass the results back to the external handler.
* NP: on errors the earlyAbort call should be cancelled first with a reason.
*/
This means that it won't be possible to cancel the call afterwards
(which may not be necessary) but documenting the 'pre calling procedure'
of some routine as "do such-and-such a thing before calling this" and
then not doing it when calling the routine seems confusing to me.
This is a case where the code was changed but the documentation left
without updating.
s/should be cancelled/ should have been cancelled/
+ ptr = static_cast<Pointer *>(F->write_data);
+ delete ptr;
This is not going to work well. F->write_data now points to deleted
memory. If you absolutely have to do this, please set it to NULL after
delete.
Insofar my understanding goes, the
Comm::SetSelect(temporaryFd_, COMM_SELECT_WRITE, NULL, NULL, 0);
in doneConnecting will do that (and - for good measure - the fd_close
called from doneConnecting will clear the write pointers again).
Yes, but the SetSelect needs to be kept paired close to the fiddling
with write_data. This is a hack, polishing it now is not a good idea.
The purpose of it is to keep the write handler state consistent. Be
conservative with the hack/workaround and don't depend on all other code
behaviour remaining the same indefinitely.
In fact, if the workaround needs to be done at more than one place of
the code it needs to be in a separate private method and documented as a
hack in the methods docs.
Amos
