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

Alex Rousskov <rousskov_at_measurement-factory.com> writes:
> 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,

To be honest, I don't really care: At worst, I would put this issue in
some todo-list file but I wouldn't at comments describing it if this
was entirely my own code. I just tried my best to distill what I
understood from Amos' statement about the workaround, namely,
'ideally', just clearing the pointers via Comm::SetSelect would free
anything which needs to be freed implicitly but since the data is
ultimatively a void * and not some a pointer/ reference to an object of
some class capable of 'freeing it' (the data), whatever 'it' happens
to be, the explicit delete is still necessary at the moment, although
this will hopefully change in future. You're - of course - right about
the inaccuracy regarding the leak.
Received on Wed Jan 23 2013 - 22:34:12 MST