From: "Ilya Kosarev" <i.kosarev@tarantool.org> To: "Vladislav Shpilevoy" <v.shpilevoy@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH v7 3/3] iproto: move greeting from tx thread to iproto Date: Tue, 22 Dec 2020 03:50:10 +0300 [thread overview] Message-ID: <1608598210.19658708@f180.i.mail.ru> (raw) In-Reply-To: <d1eb7c2a-7afa-c2d9-9069-8a761d6aba86@tarantool.org> [-- Attachment #1: Type: text/plain, Size: 4737 bytes --] Hi! Sent v8 of the patch. It includes improvement to fix the problem. >Понедельник, 21 декабря 2020, 19:20 +03:00 от Vladislav Shpilevoy <v.shpilevoy@tarantool.org>: > >Hi! Thanks for the fixes! > >Unfortunately, it seems the patch does not help. Probably it did earlier, >but now the descriptors are not closed. > >The example you use in the ticket is not really hard enough - you >use only one connection, and you treat 'fetch_schema' as success. >Also you assume the connection won't be re-created repeatedly by the >external code. > >To test it more thoroughly I used these scripts: > >Instance 1: > >box.cfg({listen = 3301}) >box.schema.user.grant('guest', 'super') >while true do os.execute("sleep 1000") end > >I used big sleep so as the instance wouldn't have a chance to handle >the pending connection requests. > >Instance 2: > >fiber = require('fiber') >netbox = require('net.box') >host = 'localhost:3301' >count = 0 > >function many_connect() >while true do >local c = netbox.connect(host, {wait_connected = false}) >while c.state ~= 'fetch_schema' do fiber.yield() end >count = count + 1 >c:close() >end >end > >f1 = fiber.new(many_connect) >f2 = fiber.new(many_connect) >f3 = fiber.new(many_connect) >f4 = fiber.new(many_connect) > >I create 4 fibers to reach the descriptor limit faster. You >can try more or less, but with big enough sleep() timeout the >result will be the same. On my machine with 4 fibers I hit the >descriptor limit (2560 in my case) almost immediately. > >Each fiber does a totally normal thing - creates a connection and >closes it. Lets assume, they also tried to execute some request, >it failed a timeout, and they decide to re-create the connection. > >If the descriptors would be closed on Instance 1, these fibers >would work fine, and 'count' would grow until Instance 1 stops >the sleep(). Or until it will hit OOM due to too many heavy >struct iproto_connection objects, which is not addressed here, but >we agreed to do it separately, so it is ok for now maybe. > >But what happens is that it prints > >SystemError accept, called on fd 16, aka [::]:3301: Too many open files > >And no new connections can be accepted. I killed Instance 2 and >started it again - it starts failing immediately. Which means the >descriptors are not closed. Yes, all true. I actually forgot that iproto has to be able to close the socket by itself. Well, now it is fixed. The idea is that we actually need to start reading immediately when the greeting is sent. Then we only need to care about not processing extra request on actually closed connection. It is achieved through patching of tx_accept_msg() and it’s callees. So in case the connections closes, the iproto_connection_close() in invoked and it closes the socket. When everything finally moves to the tx, all requests are being ignored depending on the the connection state. Fixed in v8 of the patch. > >> diff --git a/src/box/iproto.cc b/src/box/iproto.cc >> index f7330af21d..93bf869299 100644 >> --- a/src/box/iproto.cc >> +++ b/src/box/iproto.cc> @@ -1938,50 +1996,63 @@ net_end_subscribe(struct cmsg *m) >> + >> +/** >> + * Create the session and invoke the on_connect triggers. >> */ >> static void >> tx_process_connect(struct cmsg *m) >> { >> struct iproto_msg *msg = (struct iproto_msg *) m; >> struct iproto_connection *con = msg->connection; >> - struct obuf *out = msg->connection->tx.p_obuf; >> - try { /* connect. */ >> - con->session = session_create(SESSION_TYPE_BINARY); >> - if (con->session == NULL) >> - diag_raise(); >> - con->session->meta.connection = con; >> - tx_fiber_init(con->session, 0); >> - char *greeting = (char *) static_alloc(IPROTO_GREETING_SIZE); >> - /* TODO: dirty read from tx thread */ >> - struct tt_uuid uuid = INSTANCE_UUID; >> - random_bytes(con->salt, IPROTO_SALT_SIZE); >> - greeting_encode(greeting, tarantool_version_id(), &uuid, >> - con->salt, IPROTO_SALT_SIZE); >> - obuf_dup_xc(out, greeting, IPROTO_GREETING_SIZE); >> - if (! rlist_empty(&session_on_connect)) { >> - if (session_run_on_connect_triggers(con->session) != 0) >> - diag_raise(); >> - } >> - iproto_wpos_create(&msg->wpos, out); >> - } catch (Exception *e) { >> + >> + con->session = session_create(SESSION_TYPE_BINARY); >> + if (con->session == NULL) { >> tx_reply_error(msg); >> msg->close_connection = true; >> + return; >> + } >> + con->session->meta.connection = con; >> + >> + tx_fiber_init(con->session, 0); >> + if (! rlist_empty(&session_on_connect)) { > >In the new code we don't use ' ' after unary operators. Fixed in v8. > >> + if (session_run_on_connect_triggers(con->session) != 0) { >> + tx_reply_error(msg); >> + msg->close_connection = true; >> + } >> } >> } -- Ilya Kosarev [-- Attachment #2: Type: text/html, Size: 6079 bytes --]
next prev parent reply other threads:[~2020-12-22 0:50 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-12-19 17:39 [Tarantool-patches] [PATCH v7 0/3] iproto: greeting enhancement Ilya Kosarev 2020-12-19 17:39 ` [Tarantool-patches] [PATCH v7 1/3] iproto: move msg fields initialization to iproto_msg_new() Ilya Kosarev 2020-12-19 17:39 ` [Tarantool-patches] [PATCH v7 2/3] iproto: fix comment and add assert on destruction Ilya Kosarev 2020-12-19 17:39 ` [Tarantool-patches] [PATCH v7 3/3] iproto: move greeting from tx thread to iproto Ilya Kosarev 2020-12-21 16:20 ` Vladislav Shpilevoy 2020-12-22 0:50 ` Ilya Kosarev [this message] 2020-12-22 0:49 [Tarantool-patches] [PATCH v7 0/3] iproto: greeting enhancement Ilya Kosarev 2020-12-22 0:50 ` [Tarantool-patches] [PATCH v7 3/3] iproto: move greeting from tx thread to iproto Ilya Kosarev 2020-12-22 14:21 ` Vladislav Shpilevoy 2020-12-24 20:13 ` Ilya Kosarev 2020-12-26 13:15 ` Vladislav Shpilevoy
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=1608598210.19658708@f180.i.mail.ru \ --to=i.kosarev@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v7 3/3] iproto: move greeting from tx thread to iproto' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox