Re: [Tarantool-patches] [PATCH v2 1/1] iproto: don't destroy a session during disconnect

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the review!

On 16/11/2019 14:07, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/11/16 12:48]:
>> Binary session disconnect trigger yield could lead to use after
>> free of the session object. That happened because iproto thread
>> sent two requests to TX thread at disconnect:
> 
> LGTM, except that I usually try to avoid duplicating state, so
> do_destroy_on_close looks a bit superfluous, especially since you
> have another variable - is_destroy_sent.

It is not superfluous. Because 'is_destroy_sent' is not enough.

If that flag is false, it does not say anything about whether
disconnect is done. And can't be used in iproto_connection_close()
to decide if it needs to be destroyed now.

If that flag is true, it means, that destroy is already sent, and
it is too late to check for anything. Close will never be called
again.

But I could replace both flags with a 'state':
IPROTO_CON_ALIVE -> IPROTO_CON_CLOSED -> IPROTO_CON_PENDING_DESTROY,
IPROTO_CON_DESTROYED.

Alive is an initial state. Closed is when the socket dies and
disconnect is sent to TX. Pending destroy is an optional state,
equivalent of do_destroy_on_close. Destroyed is an equivalent of
is_destroy_sent.

If you think this is better, I can do that.

In terms of memory both state and flag are free anyway. Because
curently we have 7 byte unused padding between is_destroy_sent
and in_stop_list. A flag or an int (enum) fill this padding.