Re: [Tarantool-patches] [PATCH v21 6/6] test: add box.lib test

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

On Sun, Apr 11, 2021 at 05:43:02PM +0200, Vladislav Shpilevoy wrote:
> > + | ---
> > + | ...
> > +assert(old_module['debug_refs'] == 3) -- box.lib + 2 box.schema.func
> 
> 1. Why +2? There is only one function loaded from that module in
> box.schema.func.

Since schema.func loads module implicitly: for first load it increments
not only own refs but low level module as well, then one more ref comes
from function creation and finally one from new box.lib interface.

IOW

box.schema.func.create('cfunc.cfunc_add', {language = "C"})
box.func['cfunc.cfunc_add']:call({1,2})
-->
  func_c_call
    func_c_load
      schema_module_load
        schema_do_module_load
          module_load
            module_new
              module_ref(m);		// ref = 1
    func_c_load_from
      module_func_load
        module_ref(m)			// ref = 2

old_module = box.lib.load('cfunc')
-->
  lbox_module_load
    module_load
      cache_find
        module_ref(m);			// ref = 3

Now when we unload function from box.schema.func
func_c_unload
  schema_module_unref
    schema_module_delete
      module_unload			// ref = 2
  module_func_unload
    module_unref			// ref = 1

So it only kept by `old_module`.

> > +-- The box.lib instance should carry own
> > +-- references to the module and old
> > +-- function. And reloading must not
> > +-- affect old functions. Thus one for
> > +-- box.lib and one for box.lib function.
> 
> 2. What do you mean "one for box.lib"?

Typo, one for box.schema.func and one for box.lib.
> > +
> > +-- Same time the reload should update
> > +-- low level module cache, thus two
> > +-- for box and box function plus one
> 
> 3. The same question. What is "for box"?
> What exactly keeps these references?

Two entries for box.schema.func and one
for box.lib. I'll extend the comments.

> 
> > +-- new box.lib.
> > +new_module = box.lib.load('cfunc')
> > + | ---
> > + | ...
> > +assert(new_module['debug_refs'] == 3)
> > + | ---
> > + | - true
> > + | ...
> > +
> 
> 4. In the previous review I asked you to add tests
> for automatic unload via GC. Please, add them.
> 
> You can use collectgarbage('collect') functions,
> and setmetatable(..., {__mode = 'v'}) if it will
> help with anything.

Will try, thanks, Vlad!

> 
> See below my review fixes and on the branch in a
> separate commit.

Thanks, I'll merge them and send a final diff.