From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp34.i.mail.ru (smtp34.i.mail.ru [94.100.177.94]) (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 2C1FE469710 for ; Thu, 7 May 2020 10:29:04 +0300 (MSK) References: <20200506153804.5szcwvvl22eeeilr@tkn_work_nb> From: lvasiliev Message-ID: <8c0c398a-f043-0637-48f6-0d350b7a8c03@tarantool.org> Date: Thu, 7 May 2020 10:29:02 +0300 MIME-Version: 1.0 In-Reply-To: <20200506153804.5szcwvvl22eeeilr@tkn_work_nb> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH] socket: fix error while closing socket.tcp_server List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: tarantool-patches@dev.tarantool.org Hi! Thank you for the review. On 06.05.2020 18:38, Alexander Turenko wrote: >> @@ -1082,6 +1085,9 @@ local function tcp_server_loop(server, s, addr) >> fiber.name(format("%s/%s:%s", server.name, addr.host, addr.port), {truncate = true}) >> log.info("started") >> while socket_readable(s) do >> + if s._server_socket_closed then >> + break >> + end > > Can we use the same check `._gc_socket.fd >= 0` as in check_socket() > (we can wrap it to a function like socket_is_closed())? > > It seems that using the same way for determining whether a socket is > closed owuld simplify the code a bit. > > WBR, Alexander Turenko. > If I understand correctly, in this case we must not only check whether the socket is closed or not, but also check whether it was closed without errors by checking errno. In fact, we come to my first patchset, where I disable throwing an exception from the check_socket(). Disabling throwing an exception is the right way because now our API doesn't correspond to the documentation (I will create a ticket for this). But fixing a socket check leads to big changes in the code that are not related to the problem and will not pass the review. I can wrap the flag check in a method, but then all sockets should have a flag and it's look like overkill (this will duplicate the functionality of the check_socket() function after fix). In addition, we don't check EV_ERROR and it seems to me that this can also lead to the fact that the check you proposed may be insufficient.