From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [94.100.177.111]) (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 E3C1746971A for ; Tue, 10 Dec 2019 02:31:24 +0300 (MSK) References: <20191209072140.GD433@atlas> From: Vladislav Shpilevoy Message-ID: Date: Tue, 10 Dec 2019 00:31:22 +0100 MIME-Version: 1.0 In-Reply-To: <20191209072140.GD433@atlas> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 2/2] fiber: destroy fiber.storage created by iproto List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Konstantin Osipov , tarantool-patches@dev.tarantool.org, imun@tarantool.org Hi! Thanks for the review! Disclaimer: in case something looks 'toxic', this is an accident, and is not done intentionally. On 09/12/2019 08:21, Konstantin Osipov wrote: > * Vladislav Shpilevoy [19/12/08 23:47]: > > The danger of reporting, self-assigning and fixing a bug is that > ... it might be not a bug. I agree. Except when something is obviously a bug which is clearly the case. > > Users should know that iproto fibers are pooled, not > created/destroyed on demand. So fiber local storage is simply not > making any sense for them. Here I disagree. Users should not know anything about whether the fibers are pooled, when, and how. If so, then please, give an example, when such a strange fiber.storage is needed? I just really can't come up with a usage case. I did a public poll to understand what a behaviour is expected, and not a single person expects, that pooled vs not pooled fiber would affect its behaviour. Pool is like a cache. It should not be visible. You literally said it in another thread about SQL PREPARE. > > A proper fix would be to disable fiber local storage for these > fibers, not try to make it work. That would lead to a problem, that functions, using fiber.storage, would work properly being called from a background fiber, but would fail being called from an iproto-used fiber. It is clearly not a fix, but rather a much worse bug, sorry. > > Even though your patch "works", it is pretty much useless. > Well, sorry, with all respect, but I disagree here too. Appeared, there were repetitive requests to fix this weird behaviour of fiber.storage a couple of years ago, and again recently, which were successfully rejected by you and by the same reason, that pooling, as you think, should be exposed. My patch fixes that problem, what makes it not useless. Moreover, that fixes another problem about pooled fibers never freeing the storages (= leak) and therefore occupying Lua memory, which is very limited in size. That may become a real problem, when there are many pooled fibers. Their count may be thousands, and depends on net_msg_max.