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

On 18/01/2013 10:53 a.m., Rainer Weikusat wrote:
> Alex Rousskov <rousskov_at_measurement-factory.com> 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
Received on Fri Jan 18 2013 - 05:04:32 MST