From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp43.i.mail.ru (smtp43.i.mail.ru [94.100.177.103]) (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 9F36E4765E0 for ; Sat, 26 Dec 2020 16:15:45 +0300 (MSK) References: <1e82881e022f12ac8f2381c072459e838eefaff6.1608596852.git.i.kosarev@tarantool.org> <0e17b527-929c-b757-227a-f3b36c7906c5@tarantool.org> <1608840794.565118442@f186.i.mail.ru> From: Vladislav Shpilevoy Message-ID: <534c6e8d-b817-da59-97d0-d5e6a5a6ffa1@tarantool.org> Date: Sat, 26 Dec 2020 16:15:43 +0300 MIME-Version: 1.0 In-Reply-To: <1608840794.565118442@f186.i.mail.ru> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Tarantool-patches] [PATCH v7 3/3] iproto: move greeting from tx thread to iproto List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ilya Kosarev Cc: tarantool-patches@dev.tarantool.org On 24.12.2020 23:13, Ilya Kosarev wrote: >   > Hi! >   > Sent v9 with fixes. Answers below. > > Вторник, 22 декабря 2020, 17:21 +03:00 от Vladislav Shpilevoy >: >   > Thanks for the patch! > > Did you even test it? > > I used exactly the same test as in my last email and I still get > "too many open files". > > Yes, it does print this. But it also actually closes sockets > independently from tx. It does not. I couldn't connect after this message started being printed. If it would work, the variable `count` in my test would grow infinitely. On the latest version of the branch it seems to be working though. Maybe. On the same test. I didn't look at the code yet. Maybe it could break via another test. > The issue is in input stop through > iproto_connection_stop_msg_max_limit() with > iproto_check_msg_max() condition, which is not really > applicable while tx is not involved, so i changed the > condition for limitation in v9. > > > See 2 comments below. > > > diff --git a/src/box/iproto.cc b/src/box/iproto.cc > > index f7330af21d..b48a774c92 100644 > > --- a/src/box/iproto.cc > > +++ b/src/box/iproto.cc > > @@ -1484,8 +1544,16 @@ static inline struct iproto_msg * > > tx_accept_msg(struct cmsg *m) > > { > > struct iproto_msg *msg = (struct iproto_msg *) m; > > - tx_accept_wpos(msg->connection, &msg->wpos); > > - tx_fiber_init(msg->connection->session, msg->header.sync); > > + struct iproto_connection *con = msg->connection; > > + if (con->state != IPROTO_CONNECTION_ALIVE) { > > 1. Connection state can only be changed and read by iproto thread. > The variable is not protected anyhow, so you can't simply read/write > it in 2 threads. > > Why is it not fine? Do I really need to explain, why it is not fine to read and write a variable from multiple threads without any protection? The most obvious reason - you can end up reading garbage, in case something won't be right with the alignment. Non-obvious reason - the state can be checked to the bad state right after you checked that it was in a good state. That makes the check basically useless. Another non-obvious reason - memory read/write reorderings, but I don't know if they can hit anything in iproto code. Please, stop doing that. You probably did already, but I didn't look at the code, as I said. Because I am on a kind of 'vacation'.