Re: [Tarantool-patches] [PATCH v7 3/3] iproto: move greeting from tx thread to iproto

[Vladislav Shpilevoy <v.shpilevoy@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 <v.shpilevoy@tarantool.org </compose?To=v.shpilevoy@tarantool.org>>:
>      
>     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'.