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 073AD6EC5D; Wed, 7 Apr 2021 02:43:29 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 073AD6EC5D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1617752609; bh=9sCSxSdW0sCwyROaBCcPCHThEqTZjHVqECt9dLDKy70=; h=To:Cc:References:Date:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=Md+QeOMqv/mdmGJHF3YARFxfHYc4OvC9uubi5JqD0fScSFgXd7tBXErWxNEUshEr8 WkGIo856rjBguYPF36SnXgzb96dXbfGccsR7bNA7vaTPvAoqWROcXmVccFfyDUxnQM k2JhDu14DF9hCH7dUz+r6muuF7JnE9wSk+mVyCjk= 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 9CAE86EC5D for ; Wed, 7 Apr 2021 02:43:27 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 9CAE86EC5D Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1lTvLy-00077M-VA; Wed, 07 Apr 2021 02:43:27 +0300 To: Cyrill Gorcunov Cc: tml 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> Message-ID: <5bd028c8-21fe-8395-816b-1d5464aaa1ca@tarantool.org> Date: Wed, 7 Apr 2021 01:43:26 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.9.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD912A3E3D5D4B49FC11CA03434DF6EAA5F4478E741A4EA829900894C459B0CD1B9EA6D6338B77248B66F7B69C43D951D2C9C84356F77B7E7A728006A18B89A7669 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE731D82F3F177D3BCDEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637FA81DCE0280C9CC68F08D7030A58E5AD6BA297DBC24807EAA9D420A4CFB5DD3E6C5D4DE0B8D5BEB0BE0D497DCBB9B3FF733DFD74661ACE318941B15DA834481FA18204E546F3947C0CABCCA60F52D7EBF6B57BC7E64490618DEB871D839B7333395957E7521B51C2DFABB839C843B9C08941B15DA834481F8AA50765F7900637D9CFB327BC390E24389733CBF5DBD5E9B5C8C57E37DE458B9E9CE733340B9D5F3BBE47FD9DD3FB595F5C1EE8F4F765FC72CEEB2601E22B093A03B725D353964B0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E735B25CBF701D1BE873C4224003CC83647689D4C264860C145E X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A2AD77751E876CB595E8F7B195E1C97831D78EC93B74C30CE8A31E5B19C74FC7CE X-C1DE0DAB: 0D63561A33F958A56C5D4DE0B8D5BEB0BE0D497DCBB9B3FFD6AA05E6C01B88B7D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34EE19B6E2433CA093E838456EEB13C45A3A272572CDA926DE0AF36EC6AC2DDE1914D0F6CA2E00B50D1D7E09C32AA3244C0BA3D9DA7D7E268FA4BF5FD921D4FDD3A90944CA99CF22E3729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojAX27rRizpLnqVXZ5mqwmIA== X-Mailru-Sender: 689FA8AB762F73936BC43F508A0638226111CB8A889F731FA1BED47E0BFEC1343841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E267EA787935ED9F1B X-Mras: Ok 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: Vladislav Shpilevoy via Tarantool-patches Reply-To: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" On 07.04.2021 00:05, Cyrill Gorcunov wrote: > 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. It does not matter who allocated them. It is a matter of ownership. For example, we have tuples. They are allocated on slabs. But they belong to the code which called tuple_ref(). And until the ref is gone, the tuple can't be deleted. And the slabs can't simply free all the memory under the feet of the tuple owners. The same for tuple_format. It can't be deleted until its tuples are all deleted. Even if the entire process is being shutdown, deletion of the formats must happen naturally. After all the tuples are properly deleted, the formats would be gone too, automatically. The same for the other objects having a reference counter. You can't just unref it whenever you feel like it. It is the same here. The modules are owned by the code which called load/ref. Not by the module cache. The cache itself does not keep any single ref. Otherwise the modules would never be deleted, because they would have always at least one ref from the cache itself. The module cache job is to cache, not to own. Owners are the schema modules and box.lib modules. The cache **does not own**, therefore it can't just delete whatever it wants. > 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. As I said, under no circumstances you can free the objects which you do not own. > 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. I am fine with freeing the hash table itself and setting it to NULL, if you want to free something so hard. Then at least in future it would crash right away on any attempt to load/unload something after the cache was destroyed. Not at some random time due to memory corruptions if you would free the modules which you don't own and then they would be accessed. Might happen easily if we ever would allow to load the modules from C API, or would terminate Lua properly.