From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 4DADC469719 for ; Wed, 28 Oct 2020 01:06:56 +0300 (MSK) From: Vladislav Shpilevoy References: <20200925225359.29653-1-i.kosarev@tarantool.org> <903df397-8107-1619-e589-2e41c52d0478@tarantool.org> <1603100994.191990135@f429.i.mail.ru> Message-ID: Date: Tue, 27 Oct 2020 23:06:54 +0100 MIME-Version: 1.0 In-Reply-To: <1603100994.191990135@f429.i.mail.ru> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Tarantool-patches] [PATCH v3] iproto: make iproto thread more independent from tx List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ilya Kosarev Cc: tarantool-patches@dev.tarantool.org Привет. Далее будет ревью на русском, чтобы лучше понимать друг друга. > > There are some other > > cases where we might not want to spend precious tx time to process > > the connection in case iproto can do it all alone. > > 3. What are these other cases? I can think only of SWIM in iproto thread, > but it is not related at all. > > The cases are that we can now add more logic to request > processing in iproto, if needed, when tx is not really needed to answer. > For example, see #4646. > This can also solve some problems with improper tx answers. Какие improper tx answers? > > This patch allows iproto to accept connection and send greeting by > > itself. The connection is initialized in tx thread when the real > > request comes through iproto_msg_decode(). In case request type was not > > recognized we can also send reply with an error without using tx. It is > > planned to add more iproto logic to prevent extra interaction with > > tx thread. This patch already to some extent solves descriptors leakage > > problem as far as connection establishes and stays in fetch_schema > > state while tx thread is unresponsive. > > The other user visible change is that on_connect triggers won't run on > > connections that don't provide any input, as reflected in > > bad_trigger.test.py. > > > > Part of #3776 > > > > @TarantoolBot document > > Title: iproto: on_connect triggers execution > > Update the documentation for on_connect triggers to reflect that they > > are now being executed only with the first request. Though the triggers > > are still the first thing to be executed on a new connection. While it > > is quite a synthetic case to establish a connection without making > > any requests it is technically possible and now your triggers won't be > > executed in this case. Some request is required to start their > > execution. > > 4. I am not sure I understand. According to 3776 there was a problem, that > the connections were made. So they didn't do any requests? And the > case is not that syntetic as it seems? > > They didn’t make any requests just because they weren’t even greeted. > Instead they were reconnecting all the time. As soon as they are greeted > they are sending requests. Не совсем так. Да, они не были greeted, но проблема не в том, что они слали или не слали реквесты. А в том, что даже если они делали дисконнект, он должен был пройти через tx поток. После твоего патча вроде как не должен. В итоге они так же коннектятся, зависают, отключаются, но теперь закрытие сокета ловится как EV_READ в ипрото потоке, и там же закрывается. Правда объект iproto_connection, все буферы, и struct iproto_msg не удаляются все равно. Как бы это не вылетело по памяти еще раньше. Все-таки неправильно, что ты сразу шлешь запросы в tx поток. Надо чтоб TX явно говорил, что это можно делать. До тех пор даже не пытаться туда ничего слать. Если учесть, что инстанс может подниматься часами, у него может кончиться память при агрессивных реконнектах с кучи клиентов. Сейчас время на это есть, поэтому кмк лучше сделать это одним нормальным патчсетом, торопиться некуда. > What was the problem to preserve the original behaviour? For example, > you could make the on_connect execution delayed. TX thread could tell > when recovery is done, and iproto would send the pending connections to it. > > I think we don’t really need to deal with the connections that don’t send requests. Тут не про то, кто как думает, а как не сломать совместимость. Походу это ломает. Раньше триггер звался на все коннекты, и юзер мог их например отлуплять, выдав ошибку в триггере. Или как-то логгировать. Я все еще не вижу препятствий это не ломать. Пока ТХ тред не дал разрешение передавать ему запросы, отлупляем всех. Когда ТХ тред дал разрешение, все еще не закрытые коннекты идут в ТХ, даже если у них нет запросов. В чем проблема это сделать и не ломать то, что уже есть? > > const char *errmsg; > > while (con->parse_size != 0 && !stop_input) { > > if (iproto_check_msg_max()) { > > @@ -1314,13 +1367,24 @@ iproto_msg_decode(struct iproto_msg *msg, const char **pos, const char *reqend, > > (uint32_t) type); > > goto error; > > } > > - return; > > + return 0; > > error: > > /** Log and send the error. */ > > diag_log(); > > - diag_create(&msg->diag); > > - diag_move(&fiber()->diag, &msg->diag); > > - cmsg_init(&msg->base, error_route); > > + if (msg->connection->session != NULL) { > > + diag_create(&msg->diag); > > + diag_move(&fiber()->diag, &msg->diag); > > + cmsg_init(&msg->base, error_route); > > + return 1; > > 10. Why do you need 3 return values? You only check for -1. So you > could return 0 here like it was before, and -1 below. In the > calling code you would check != 0 like we do everywhere. > > Well, the idea was that we have 2 different error cases. But actually > it should be: 0 if the route was inited. -1 otherwise. Now it is like that. > Fixed in v4. Идея понятна, что там разное может внутри произойти. Но ты использовал только 2 случая. Какой смысл в третьем? Это был просто мертвый код. Добавлять его "на всякий случай" нет никакого смысла. > > + } > > + /* > > + * In case session was not created we can process error path > > + * without tx thread. > > + */ > > + tx_accept_wpos(msg->connection, &msg->wpos); > > + tx_reply_error(msg); > > 11. There is a naming convention, that all tx_ functions are called in > TX thread always. All net_ and iproto_ functions are called in IProto > thread. Lets not violate the rule and obfuscate the iproto.cc code > even more that it is. Please, come up with new names reflecting where > the functions are used. Or at least not specifying it as tx_. > > Well, this convention is already broken to some extent. For example, > tx_reply_error() itself calls iproto_reply_error() and > iproto_wpos_create(). tx_process_sql() calls iproto_reply_sql(). Возможно я напиздел. Скорее всего iproto_ - это общие функции. net_ - для сетевого потока, и tx_ - для главного потока. С некоторыми исключениями, которые скорее всего просто неправильны. > > +{ > > + if (!msg->close_connection) > > + return false; > > + struct iproto_connection *con = msg->connection; > > + int64_t nwr = sio_writev(con->output.fd, msg->wpos.obuf->iov, > > + obuf_iovcnt(msg->wpos.obuf)); > > 16. You can't just write to a socket whenever you want. It may be > not writable. Please, use libev events and callbacks to send anything. > You can do a write only when you get EV_WRITE event from libev, who > in turn gets it from select/poll/epoll/kqueue/whatever-else-is-provided > by the OS. > > Right. As discussed in voice, it is special error case. In case we will > do the normal way, we won’t be able to send the error. I checked the > case with bad_trigger.test and it clearly shows that the client won’t > receive  the error message in case we will try to wait for the event here, > while sio_writev() does its job. Ясен пень, если ты сокет закроешь, то писать в него будет нельзя, и никакие события на него больше не придут от libev. Надо его не закрывать, и тогда все будет. Голосом мы обсудили, что это не специальный случай, когда записать ошибку нормально невозможно, а что просто автору оригинального кода было лень сделать это нормально через события. И он решил записать в сокет напрямую. Теперь это протаскивается дальше. Еще вероятная причина - все заточено на использование obuf, в который нельзя было писать в ипрото потоке впринципе. Я пока не могу сказать, что с этим делать. Зависит от того, что дальше будет с obuf. См другое письмо.