From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <vdavydov.dev@gmail.com>
Date: Wed, 5 Dec 2018 11:52:05 +0300
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Subject: Re: [tarantool-patches] Re: [PATCH 06/11] evio: make on_accept be
 nothrow
Message-ID: <20181205085205.t55ec3fn7jwvlbkd@esperanza>
References: <cover.1543590433.git.v.shpilevoy@tarantool.org>
 <5aa67f292cac37f8b9d2f77b97221b81b1513195.1543590433.git.v.shpilevoy@tarantool.org>
 <20181203145828.iholwtaejm2arnib@esperanza>
 <3762b2cc-ef59-47ec-d967-604af6edd7b5@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <3762b2cc-ef59-47ec-d967-604af6edd7b5@tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@freelists.org
List-ID: <tarantool-patches.dev.tarantool.org>

On Wed, Dec 05, 2018 at 12:29:14AM +0300, Vladislav Shpilevoy wrote:
> 
> 
> 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.

OK, I see. Turns out the iproto_on_accept function doesn't raise an
exception on error, which isn't correct from the point of on_accept
callback protocol. I didn't notice that. Worth mentioning in the
commit message.

> 
> > 
> > > @@ -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.

OK.