Re: [Tarantool-patches] [PATCH v20 4/7] box/module_cache: introduce modules subsystem

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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.