

<HTML><BODY><div class="js-helper js-readmsg-msg"><div><div id="style_16086651832010482138_BODY"><div class="cl_843717"><div> </div></div></div></div></div><div>Hi!<br> </div><div>Sent v9 with fixes. Answers below.</div><div class="js-helper js-readmsg-msg"><div><div><div class="cl_843717"><div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;">Вторник, 22 декабря 2020, 17:21 +03:00 от Vladislav Shpilevoy <<a href="/compose?To=v.shpilevoy@tarantool.org">v.shpilevoy@tarantool.org</a>>:<br> <div id=""><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div id="style_16086468640714360343_BODY_mr_css_attr">Thanks for the patch!<br><br>Did you even test it?<br><br>I used exactly the same test as in my last email and I still get<br>"too many open files".</div></div></div></div></blockquote></div><div>Yes, it does print this. But it also actually closes sockets</div><div>independently from tx. The issue is in input stop through</div><div>iproto_connection_stop_msg_max_limit() with</div><div>iproto_check_msg_max() condition, which is not really</div><div>applicable while tx is not involved, so i changed the</div><div>condition for limitation in v9.</div><div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div><br>See 2 comments below.<br><br>> diff --git a/src/box/iproto.cc b/src/box/iproto.cc<br>> index f7330af21d..b48a774c92 100644<br>> --- a/src/box/iproto.cc<br>> +++ b/src/box/iproto.cc<br>> @@ -1484,8 +1544,16 @@ static inline struct iproto_msg *<br>> tx_accept_msg(struct cmsg *m)<br>> {<br>> struct iproto_msg *msg = (struct iproto_msg *) m;<br>> - tx_accept_wpos(msg->connection, &msg->wpos);<br>> - tx_fiber_init(msg->connection->session, msg->header.sync);<br>> + struct iproto_connection *con = msg->connection;<br>> + if (con->state != IPROTO_CONNECTION_ALIVE) {<br><br>1. Connection state can only be changed and read by iproto thread.<br>The variable is not protected anyhow, so you can't simply read/write<br>it in 2 threads.</div></div></div></div></blockquote></div></div></div></div></div><div>Why is it not fine? I think it may lead to false sharing problem, but</div><div>as far as i see it is not going to affect iproto thread, as long as we</div><div>are not writing connection state in tx, only reading.</div><div class="js-helper js-readmsg-msg"><div><div><div class="cl_843717"><div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div><br>> + /*<br>> + * Connection might be closed from iproto already.<br>> + * No action required in this case.<br>> + */<br>> + return msg;<br><br>2. Why do you return a message instead of NULL here, if the<br>connection is already dead?</div></div></div></div></blockquote></div></div></div></div></div><div>Ok, this can be simplified, fixed in v9.</div><div class="js-helper js-readmsg-msg"><div><div><div class="cl_843717"><div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div><br>I ask you to spend more time on finishing the patch. If you will<br>rush to "fix" it without thinking it through, the review will<br>never end, and it will waste time both yours and mine.</div></div></div></div></blockquote> <div> </div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Ilya Kosarev</div></div></div><div> </div></div></div></div></div></div><div> </div></BODY></HTML>