

<HTML><BODY><div><div>Hi!</div><div> </div><div>Thanks for your review.<br> </div><div>Sent v7 considering your comments. Some answers below.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Среда, 16 декабря 2020, 2:04 +03:00 от Vladislav Shpilevoy <v.shpilevoy@tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16080734830447917297_BODY">Hi! Thanks for the patch!<br><br>See 4 comments below.<br><br>On 13.12.2020 00:38, Ilya Kosarev wrote:<br>> On connection, an evio service callback is invoked to accept it. The<br>> next step after acceptance was to process connection to tx thread<br>> through cbus. This meant that any connection interaction involves tx<br>> thread even before we get to send the greeting to the client.<br>> Consequently, the client might reach the instance with enormous number<br>> of connections, leading to the file descriptors limit exhaustion in<br>> case of unresponsive tx thread (for example, when building secondary<br>> index) and extra tx_process_connects afterwords, even in case the<br>> instance doesn't fail with too many open files error.<br>> This patch allows iproto to accept connection and send greeting by<br>> itself. Thus the connection is being established and stays in<br>> fetch_schema state while tx thread is unresponsive.<br>><br>> Closes #3776<br>> ---<br>> src/box/iproto.cc | 118 +++++++++++++-----<br>> test/app/gh-4787-netbox-empty-errmsg.result | 18 ---<br>> test/app/gh-4787-netbox-empty-errmsg.test.lua | 8 --<br>> 3 files changed, 88 insertions(+), 56 deletions(-)<br>><br>> diff --git a/src/box/iproto.cc b/src/box/iproto.cc<br>> index f7330af21d..a7da115925 100644<br>> --- a/src/box/iproto.cc<br>> +++ b/src/box/iproto.cc<br>> @@ -1091,6 +1093,46 @@ iproto_connection_on_output(ev_loop *loop, struct ev_io *watcher,<br>> }<br>> }<br>><br>> +static int<br>> +iproto_buf_flush(struct iproto_connection *con)<br><br>1. The function lacks a comment. It is not so trivial why do we<br>have 2 flush functions. Also maybe worth renaming it to<br>iproto_connection_greeting_flush. Because it is a method of<br>'iproto_connection' (thus the prefix), and works with the<br>greeting only.</div></div></div></div></blockquote></div><div>Right, fixed in v7 of the patch.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>> +{<br>> + struct iovec greeting;<br>> + greeting.iov_base = &con->greeting_buf;<br>> + greeting.iov_len = IPROTO_GREETING_SIZE;<br>> + ssize_t nwr = sio_writev(con->output.fd, &greeting, 1);<br>> +<br>> + if (nwr > 0) {<br>> + /* Count statistics */<br>> + rmean_collect(rmean_net, IPROTO_SENT, nwr);<br>> + return 1;<br>> + } else if (nwr < 0 && !sio_wouldblock(errno)) {<br>> + return -1;<br>> + }<br>> +<br>> + return 0;<br>> +}<br>> +<br>> +static void<br>> +iproto_connection_on_greeting(ev_loop *loop, struct ev_io *watcher,<br>> + int /* revents */)<br>> +{<br>> + struct iproto_connection *con =<br>> + (struct iproto_connection *)watcher->data;<br>> + int rc = iproto_buf_flush(con);<br>> + if (rc <= 0) {<br>> + if (rc == 0) {<br>> + ev_io_start(loop, &con->output);<br>> + } else {<br>> + diag_log();<br>> + con->state = IPROTO_CONNECTION_CLOSED;<br><br>2. I don't understand how this part works. You didn't close the<br>socket, and didn't delete it from libev. But IPROTO_CONNECTION_CLOSED<br>clearly states<br><br>/**<br>* Socket was closed, a notification is sent to the TX<br>* thread to close the session.<br>*/<br><br>The way you change it here does not match the description. AFAIU,<br>the only legal function able to set this state is<br>iproto_connection_close(), and it should remain so.<br><br>Moreover, I see this should crash in an assertion in<br>iproto_connection_close(). Because this function will be called by<br>net_finish_connect() due to state being CLOSED (which is weird -<br>why is close() called if the state is already CLOSED?). But inside<br>it has "assert(con->state == IPROTO_CONNECTION_ALIVE)" in case the<br>ev_watcher still has a descriptor - it still has, so it will go there<br>any crash.<br><br>Did I miss something?</div></div></div></div></blockquote></div><div>Yes, my bad. It definitely needs new state. There was not going to be</div><div>an assertion fail as long as i switch it back, but it is still not good. Fixed in v7.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>> + }<br>> + return;<br>> + }<br>> + ev_io_stop(con->loop, &con->output);<br>> + ev_io_init(&con->output, iproto_connection_on_output,<br>> + con->output.fd, EV_WRITE);<br>> +}<br><br>3. What if sio_writev() has written only half of the bytes?<br>You didn't check and immediately proceeded to the normal<br>operation mode. But the greeting wasn't sent yet. Not all.<br><br>Also you don't propagate iov_base in case only a part of<br>the data was sent.<br><br>You can easily check it using a temporary error injection.<br>Before sio_writev() try to inject iov_len =<br>max(iov_len, IPROTO_GREETING_SIZE / 2) to force it to work<br>in 2 iterations, and you will see the issue.<br><br>Partial write is a rare thing, but it is totally real. It<br>is not some kind of an impossible thing only from the books.<br>Even if it is just 128 bytes.</div></div></div></div></blockquote></div><div>Fixed in v7.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>> +<br>> static struct iproto_connection *<br>> iproto_connection_new(int fd)<br>> {<br>> @@ -1938,50 +1980,64 @@ net_end_subscribe(struct cmsg *m)<br>> }<br>><br>> /**<br>> - * Handshake a connection: invoke the on-connect trigger<br>> - * and possibly authenticate. Try to send the client an error<br>> - * upon a failure.<br>> + * Handshake a connection: send greeting for it.<br>> + */<br>> +static void<br>> +iproto_process_connect(struct iproto_msg *msg)<br><br>4. Why do you take iproto_msg object if the only thing you do<br>with it is take the connection object? Wouldn't it be simpler<br>and more understandable, if it would be iproto_connection_start()<br>or something like that? With an iproto_connection * argument.</div></div></div></div></blockquote></div><div>Fixed in v7.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>> +{<br>> + struct iproto_connection *con = msg->connection;<br>> + /*<br>> + * INSTANCE_UUID is guaranteed to be inited before this moment.<br>> + * We start listening either in local_recovery() or bootstrap().<br>> + * The INSTANCE_UUID is ensured to be inited in the beginning of<br>> + * both methods. In case of local_recovery() it is verified that<br>> + * INSTANCE_UUID was read from the snapshot in memtx_engine_new().<br>> + * In bootstrap() INSTANCE_UUID is either taken from the<br>> + * instance_uuid box.cfg{} param or created on the spot.<br>> + */<br>> + random_bytes(con->salt, IPROTO_SALT_SIZE);<br>> + greeting_encode(con->greeting_buf, tarantool_version_id(),<br>> + &INSTANCE_UUID, con->salt, IPROTO_SALT_SIZE);<br>> + assert(evio_has_fd(&con->output));<br>> + ev_feed_event(con->loop, &con->output, EV_WRITE);<br>> +}</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></BODY></HTML>