From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [tarantool-patches] Re: [PATCH 06/11] evio: make on_accept be nothrow References: <5aa67f292cac37f8b9d2f77b97221b81b1513195.1543590433.git.v.shpilevoy@tarantool.org> <20181203145828.iholwtaejm2arnib@esperanza> From: Vladislav Shpilevoy Message-ID: <3762b2cc-ef59-47ec-d967-604af6edd7b5@tarantool.org> Date: Wed, 5 Dec 2018 00:29:14 +0300 MIME-Version: 1.0 In-Reply-To: <20181203145828.iholwtaejm2arnib@esperanza> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit To: Vladimir Davydov Cc: tarantool-patches@freelists.org List-ID: On 03/12/2018 17:58, Vladimir Davydov wrote: > On Fri, Nov 30, 2018 at 06:39:38PM +0300, Vladislav Shpilevoy wrote: >> diff --git a/src/box/iproto.cc b/src/box/iproto.cc >> index 07ef23cac..dd76e28bd 100644 >> --- a/src/box/iproto.cc >> +++ b/src/box/iproto.cc >> @@ -1800,26 +1800,23 @@ iproto_on_accept(struct evio_service * /* service */, int fd, >> struct iproto_msg *msg; >> struct iproto_connection *con = iproto_connection_new(fd); >> if (con == NULL) >> - goto error_conn; >> + return -1; >> /* >> * Ignore msg allocation failure - the queue size is >> * fixed so there is a limited number of msgs in >> * use, all stored in just a few blocks of the memory pool. >> */ >> msg = iproto_msg_new(con); >> - if (msg == NULL) >> - goto error_msg; >> + if (msg == NULL) { >> + mempool_free(&iproto_connection_pool, con); >> + return -1; >> + } >> cmsg_init(&msg->base, connect_route); >> msg->p_ibuf = con->p_ibuf; >> msg->wpos = con->wpos; >> msg->close_connection = false; >> cpipe_push(&tx_pipe, &msg->base); >> - return; >> -error_msg: >> - mempool_free(&iproto_connection_pool, con); >> -error_conn: >> - close(fd); >> - return; >> + return 0; > > You don't close the file descriptor on error anymore. I guess it's OK, > because evio_service_accept_cb will close it anyway. Yes, I do not close it since evio_service_accept_cb closes it now. Before my patch the socket was closed inside iproto_on_accept because it was exception free. Evio never closed a socket, accepted by iproto_on_accept. Now a necessity to close a socket is determined by returned value instead of an exception and I can not leave this close here. > >> @@ -612,14 +612,12 @@ coio_service_on_accept(struct evio_service *evio_service, >> "%s/%s", evio_service->name, sio_strfaddr(addr, addrlen)); >> >> /* Create the worker fiber. */ >> - struct fiber *f; >> - try { >> - f = fiber_new_xc(fiber_name, service->handler); >> - } catch (struct error *e) { >> - error_log(e); >> + struct fiber *f = fiber_new(fiber_name, service->handler); >> + if (f == NULL) { >> + diag_log(); >> say_error("can't create a handler fiber, dropping client connection"); >> evio_close(loop(), &coio); > > However, you do close fd here, via evio_close(). Care to remove it? > It is a separate bug that I found and fixed in a next commit. This patch does not fix any bugs. It is pure refactoring. In v2 the patch is left as is.