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

[Ilya Kosarev]

Hi!
 
Sent v9 with fixes. Answers below.
>Вторник, 22 декабря 2020, 17:21 +03:00 от Vladislav Shpilevoy < v.shpilevoy at 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
 
 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20201224/a782fedc/attachment.html>