From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-f193.google.com (mail-lj1-f193.google.com [209.85.208.193]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 69041469719 for ; Thu, 19 Mar 2020 21:09:15 +0300 (MSK) Received: by mail-lj1-f193.google.com with SMTP id q19so3551044ljp.9 for ; Thu, 19 Mar 2020 11:09:15 -0700 (PDT) Date: Thu, 19 Mar 2020 21:09:12 +0300 From: Konstantin Osipov Message-ID: <20200319180912.GA3662@atlas> References: <17a0bca9bc72901c99f75bd14913d54de10781bd.1581500169.git.georgy@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <17a0bca9bc72901c99f75bd14913d54de10781bd.1581500169.git.georgy@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v4 03/11] coio: do not allow parallel usage of coio List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Georgy Kirichenko Cc: tarantool-patches@dev.tarantool.org * Georgy Kirichenko [20/02/12 13:09]: > Simultaneous usage of one coio from two or more fiber could lead > to undefined behavior as coio routines are replacing awaiting fiber > (a data member) and stopping watcher without any relevance if there > any other users of the coio object. Such behavior could lead to > an applier invalid stream issue #4040. > The proposal is to disable an active coio reuse by returning a fake > EINPROGRESS error. I am not aware of any cases when coio is used by multiple fibers: it's a violation of the coio contract. If you suspect it is violated in some cases, please add the scenario description to the commit comment. Then, to better identify the case, your patch should add an assert in debug mode. For release mode, I suggest we set a more clear error so that it's easy then to spot in the log file and act upon, not EINPROGRESS. Something like ERR_OPEN_A_BUG. > > Part of #980 > --- > src/lib/core/coio.cc | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/src/lib/core/coio.cc b/src/lib/core/coio.cc > index e88d724d5..faa7e5bd5 100644 > --- a/src/lib/core/coio.cc > +++ b/src/lib/core/coio.cc > @@ -238,6 +238,17 @@ coio_connect_timeout(struct ev_io *coio, struct uri *uri, struct sockaddr *addr, > tnt_raise(SocketError, sio_socketname(coio->fd), "connection failed"); > } > > +/* Do not allow to reuse coio by different fiber. */ > +static inline void > +check_coio_in_use(struct ev_io *coio) > +{ > + if (ev_is_active(coio)) { > + errno = EINPROGRESS; > + tnt_raise(SocketError, sio_socketname(coio->fd), > + "already in use"); > + } > +} > + > /** > * Wait a client connection on a server socket until > * timedout. > @@ -249,6 +260,7 @@ coio_accept(struct ev_io *coio, struct sockaddr *addr, > ev_tstamp start, delay; > coio_timeout_init(&start, &delay, timeout); > > + check_coio_in_use(coio); > CoioGuard coio_guard(coio); > > while (true) { > @@ -302,6 +314,7 @@ coio_read_ahead_timeout(struct ev_io *coio, void *buf, size_t sz, > > ssize_t to_read = (ssize_t) sz; > > + check_coio_in_use(coio); > CoioGuard coio_guard(coio); > > while (true) { > @@ -399,6 +412,7 @@ coio_write_timeout(struct ev_io *coio, const void *buf, size_t sz, > ev_tstamp start, delay; > coio_timeout_init(&start, &delay, timeout); > > + check_coio_in_use(coio); > CoioGuard coio_guard(coio); > > while (true) { > @@ -461,6 +475,7 @@ coio_writev_timeout(struct ev_io *coio, struct iovec *iov, int iovcnt, > struct iovec *end = iov + iovcnt; > ev_tstamp start, delay; > coio_timeout_init(&start, &delay, timeout); > + check_coio_in_use(coio); > CoioGuard coio_guard(coio); > > /* Avoid a syscall in case of 0 iovcnt. */ > @@ -518,6 +533,7 @@ coio_sendto_timeout(struct ev_io *coio, const void *buf, size_t sz, int flags, > ev_tstamp start, delay; > coio_timeout_init(&start, &delay, timeout); > > + check_coio_in_use(coio); > CoioGuard coio_guard(coio); > > while (true) { > @@ -563,6 +579,7 @@ coio_recvfrom_timeout(struct ev_io *coio, void *buf, size_t sz, int flags, > ev_tstamp start, delay; > coio_timeout_init(&start, &delay, timeout); > > + check_coio_in_use(coio); > CoioGuard coio_guard(coio); > > while (true) { > -- > 2.25.0 -- Konstantin Osipov, Moscow, Russia