From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 2C8706EC5D; Wed, 7 Apr 2021 01:05:56 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 2C8706EC5D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1617746756; bh=L8x+Am9T7Turrxk0xCectKuTpw2YIt7j/CPZfuybx4s=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=tv5yjqWnigCvgh2h+z607eRBuGQv7MPA7n94ii7nCHc+HTgzCJmRt/OSOwBHMhuj9 dMe+Zm1J5UEGSdUE/+yVhRrb6AEmXTc7StDDKsL/RrzrRbHCAsSFGORB0P4dHMwkWQ s6a8X9jSplm/zo1odhOMyMvrM8yWxPEMWMjJu5h8= Received: from mail-lf1-f42.google.com (mail-lf1-f42.google.com [209.85.167.42]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id D99B06EC5D for ; Wed, 7 Apr 2021 01:05:53 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org D99B06EC5D Received: by mail-lf1-f42.google.com with SMTP id v140so7344575lfa.4 for ; Tue, 06 Apr 2021 15:05:53 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=P6sBZizSJnt0QMRMhU5rB2Loe3wwF7o9S+RFs0+WFoE=; b=KiHF6V/U4JT41dPPJmWjJcbGLN+wnVvY3sSucidMj60YabcPJs6t+dliiFNZICIj6d bVIJAykWN+uDXGX7zHqNwgH9VaN26UYzp4r74G5ua7b+SagJgbqm/ZbXz2pKne50tmV+ Y2k5ZSrHf/eqBPNL7bbtUoKUkpid7+drWmGpQOK+VuOsFWw5g6lqEGTQ1RofohhGusGu kSd8ZvkQuMLt8n74RAQviczH1ePadnAJAskcSadusndtoSVtLAsCm9l5/bi9SvEt1UJT SKJ1GwY0qJw6kp0lHkuA/ImRYgeVeY9cy4HCM/tmn0ls7hR9w3bbWXKx1weAAkDHeNYh 8NSw== X-Gm-Message-State: AOAM530L2KQZucVBM4EaAmxxorKwn/gD+1ID6xdF6PptM4cT5DQnGQGl Chw6cvTtgxJIC1sjaj2/OEynStzGBprHLA== X-Google-Smtp-Source: ABdhPJxvXGqaDAB3tihwulUb9ChsNKIO9qDtJyKYlPYyDRR46Db3FaNQPPxl5g/Na3OYRI53LxWO8w== X-Received: by 2002:ac2:4c29:: with SMTP id u9mr199973lfq.164.1617746752509; Tue, 06 Apr 2021 15:05:52 -0700 (PDT) Received: from grain.localdomain ([5.18.199.94]) by smtp.gmail.com with ESMTPSA id j22sm2284968lfh.31.2021.04.06.15.05.49 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 06 Apr 2021 15:05:51 -0700 (PDT) Received: by grain.localdomain (Postfix, from userid 1000) id 4D6735601DD; Wed, 7 Apr 2021 01:05:49 +0300 (MSK) Date: Wed, 7 Apr 2021 01:05:49 +0300 To: Vladislav Shpilevoy Cc: tml Message-ID: References: <20210402123420.885834-1-gorcunov@gmail.com> <20210402123420.885834-5-gorcunov@gmail.com> <14b1c3fa-da30-e7f1-6d17-32e575d71272@tarantool.org> <7a7e6003-4aac-ac7f-228d-b6f723227ea9@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <7a7e6003-4aac-ac7f-228d-b6f723227ea9@tarantool.org> User-Agent: Mutt/2.0.5 (2021-01-21) Subject: Re: [Tarantool-patches] [PATCH v20 4/7] box/module_cache: introduce modules subsystem X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Cyrill Gorcunov via Tarantool-patches Reply-To: Cyrill Gorcunov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" On Tue, Apr 06, 2021 at 10:09:20PM +0200, Vladislav Shpilevoy wrote: > >>> +void > >>> +module_free(void) > >>> +{ > >>> + mh_int_t e; > >>> + > >>> + mh_foreach(module_cache, e) { > >>> + struct module *m = mh_strnptr_node(module_cache, e)->val; > >>> + module_unload(m); > >> > >> 5. As I said in the previous review, it does not make much sense. > >> If there are any not unloaded modules, and they try to unload later, > >> they will see module_cache == NULL and will crash. > >> > >> Also you can't do unload here, because the module_cache itself does > >> not keep any references. All the unloads must be done by the module > >> objects owners. Not by module_cache on its own. For example, if there > >> is a module having a single reference and used in some other subsystem, > >> your unload will free it and make it memory invalid. That will crash > >> in case the module owner will try to access it again. > >> > >> There should be a panic-check that the module cache is empty already. > > > > Not at all. You can exit tarantool via Ctrl+D inside console and > > modules won't be empty and we should clean them up. So I can and > > I should unload modules here. Vlad, this is _exit_ path called when > > we're exiting tarantool. What I'm missing? > > Well, if there are modules in Lua, they might have more than 1 reference, > and your module_unload won't free them anyway. But that does not matter > much as you try to free the objects which don't belong to you. The > refs do not belong to the module_cache subsystem. They belong to the > callers of module_load. > > That is a bug. Freeing what does not belong to you. I do not agree here, since objects are belong to this subsystem, and subsystem allocates them. And the bug is rather in caller side which should had install some hooks to detect exits and unref objects. But as you pointed below Lua is not properly terminated and the subsystem does only thing it knows about -- unref objects it has allocated (we setup a first ref upon allocation). It is still somehow ugly because of potential extra refs on Lua side and I now think maybe we should free allocated memory in a force way. But that's true that even though we won't have a clean exit. I tend to agree that simply free and zap the hash table is best what we could do for now. Will update. > And since the Lua land is not properly terminated with freeing all the > references, the only valid way you have here is not to do anything at > all AFAIS. Or free the hash table + set it to NULL so it would at least > crash in a sane way in case we ever start freeing all Lua refs. But > under no circumstances can you unref the modules which you didn't ref. > They were referenced by schema modules and box.lib modules, and > therefore must be unreferenced by them.