Hi!
 
Sent v9 with fixes. Answers below.
Вторник, 22 декабря 2020, 17:21 +03:00 от Vladislav Shpilevoy <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. 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? I think it may lead to false sharing problem, but
as far as i see it is not going to affect iproto thread, as long as we
are not writing connection state in tx, only reading.

> + /*
> + * Connection might be closed from iproto already.
> + * No action required in this case.
> + */
> + return msg;

2. Why do you return a message instead of NULL here, if the
connection is already dead?
Ok, this can be simplified, fixed in v9.

I ask you to spend more time on finishing the patch. If you will
rush to "fix" it without thinking it through, the review will
never end, and it will waste time both yours and mine.
 
 
--
Ilya Kosarev