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

[Cyrill Gorcunov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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.