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

On 01/21/2013 09:53 AM, Rainer Weikusat wrote:

> + // XXX: connect method installed a write handler
> + // after a successful non-blocking connect.
> + // Since this handler is not going to be called
> + // and Comm::SetSelect is not (yet) 'smart pointer
> + // aware', the Pointer object needs to be
> + // destroyed explicitly here to prevent the ConnOpener
> + // object from being leaked.
> + //

I do not think the above is 100% accurate, on two counts: We are
deleting ptr because we are about to remove our callback that deletes
the ptr. This is not directly related to SetSelect not being aware of
smart pointers (whatever that means) -- ptr is not smart. Also, it is
the ptr that leaks. The ConnOpener object leak is just a side effect of
the ptr leak.

I suggest the following phrasing:

XXX: We are about to remove write_handler, which was responsible for
deleting write_data, so we have to delete write_data ourselves. Comm
currently calls SetSelect handlers synchronously so if write_handler is
set, we know it has not been called yet. ConnOpener converts that sync
call into an async one, but only after deleting ptr, so that is not a
problem.

If you agree that this phrasing is better, it can be changed when the
patch is applied, no need to resubmit. The tail starting with "Comm
currently ..." explains why our async conversion is not a problem. That
question bothers me every time I look at this code, so I wanted to add
an answer, but you may strip that answer if you do not like it.

I do not see any other problems with this patch, but it is difficult for
me to grasp all the interactions because of the other problems remaining
in the code. For example, the timeout is still not cleared correctly on
successful connect. I am OK with this going in since Rainer prefers to
fix one problem at a time. I just hope we will remember/see all of the
remaining problems as this work progresses :-).

Personally, I would prefer a single commit fixing all known problems,
but that is not a strong preference.

Thanks a lot,

Alex.
Received on Tue Jan 22 2013 - 16:16:19 MST