From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp62.i.mail.ru (smtp62.i.mail.ru [217.69.128.42]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id AA297452566 for ; Sun, 17 Nov 2019 20:41:36 +0300 (MSK) References: <746ed8a74c07907f85db8ec08e9d1937afc44f08.1573864467.git.v.shpilevoy@tarantool.org> <20191116130740.GF14490@atlas> From: Vladislav Shpilevoy Message-ID: <600a3a99-532c-7521-aa66-e96c11e76987@tarantool.org> Date: Sun, 17 Nov 2019 18:47:59 +0100 MIME-Version: 1.0 In-Reply-To: <20191116130740.GF14490@atlas> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2 1/1] iproto: don't destroy a session during disconnect List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Konstantin Osipov , tarantool-patches@dev.tarantool.org Hi! Thanks for the review! On 16/11/2019 14:07, Konstantin Osipov wrote: > * Vladislav Shpilevoy [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.