[tarantool-patches] Re: [PATCH 06/11] evio: make on_accept be nothrow
Vladimir Davydov
vdavydov.dev at gmail.com
Wed Dec 5 11:52:05 MSK 2018
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.
More information about the Tarantool-patches
mailing list