Re: [Tarantool-patches] [PATCH v10 3/4] box/cbox: implement cbox Lua module

[Cyrill Gorcunov <gorcunov@gmail.com>]

On Thu, Nov 12, 2020 at 11:53:45PM +0100, Vladislav Shpilevoy wrote:
> > 
> > Still people would like to have a way to run such functions
> > even in ro mode. For this sake we implement "cbox" lua module.
> > 
> > Fixes #4692
> 
> 1. Wrong issue number.

Thanks, typo, fixed.

> 
> > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> > 
> > @TarantoolBot document
> > Title: cbox module
> > 
> > Overview
> > ========
> > 
> > `cbox` module provides a way to create, delete and execute
> > `C` procedures. Unlinke `box.schema.func` functionality this
> > the functions created with `cbox` help are not persistent and
> 
> 2. "functionality this the functions created"? Couldn't parse.
> "this" seems to be unnecessary.

Thanks! Changed to

    Overview
    ========
    
    `cbox` module provides a way to create, delete and execute
    `C` procedures. Unlinke `box.schema.func` the functions created
    with `cbox` help are not persistent and live purely in memory.
    Once a node get turned off they are vanished. An initial purpose
    for them is to execute them on nodes which are running in
    read-only mode.

> > `cbox.func.load([dso.]name) -> obj | nil, err`
> > ----------------------------------------------
> > 
> > Loads a new function with name `name` from module `dso.`.
> > The module name is optional and if not provided implies
> > to be the same as `name`.
> > 
> > The `load` call must be paired with `unload` and these
> > calls are accounted. Until coupled `unload` is called
> > the instance is present in memory. Any `load` calls
> > followed by another `load` with same name simply
> > increase a reference to the existing function.
> 
> 3. I thought about it more, and I am not sure unload should
> be called only manually. Because Lua has exceptions. So the
> user need to wrap everything into pcall between he called
> load() and the code calling unload(). Otherwise an exception
> may arise somewhere, and the user won't get to unload(). As
> a result, he won't decrease the reference counter.
> 
> For example:
> 
> 	local f = cbox.load('func')
> 	box.space.test:insert({f()})
> 	cbox.unload('func')
> 
> If the insert will throw, unload is never called. Moreover, if
> the code is called repeatedly, and often fails on insertion,
> then the reference counter will constantly grow, and number of
> leaked unload() calls will grow as well.
> 
> Also it looks ugly that 'unload' is not bound anyhow to the function
> objects. In Lua we usually provide a method to destroy cdata object
> content in the object itself. Not in the module function-set.
> 
> For example, look at fio. When we create a file handle via
> 
> 	fh = fio.open('file_name')
> 
> , we then call
> 
> 	fh:close()
> 
> , not
> 
> 	fio.close('file_name')
> 
> Also if :close() is not called, and 'fh' is garbage-collected, it is
> closed automatically.
> 
> Your API for cbox looks exactly like the second case. While IMO
> it should look like the first one. Otherwise the users will leak the
> modules, 100%.
> 
> On the other hand, if we will unload the function when its last
> Lua object is deleted, then we may get oscillation in case the
> user frequently gets the function object using cbox.load(). So
> maybe even if the last function is unloaded, we must keep the
> module loaded, but with 0 reference counter. This, in turn, may
> lead to the module never being deleted, if the user will not ever
> use it or reload. I don't think I have a ready to use solution for that.
> 
> I have a raw idea how to resolve that. We can separate function load
> and function get from each other. So cbox.load() only does the load
> and returns nothing. And to actually get the function we add something
> like cbox.get/fetch.

What you're describing is rather module interface. You know I though of
redesigning the modules more deeply: currently we dont even check if
module exist when create a new function. Thus if there is no such .so
file we won't hit an error until we run first :call method. And we've
been talkin to Mons a couple of weeks or month ago, he pointed that
such behaviour is really bad designed. And I completely agree.

I think we rather need something like

cbox.module.load(path)
	if module is already in cache we simply return EEXIST
cbox.module.unload(path)
	-EBUSY if someone is using it
	-ENOENT if not found
cbox.module.reload(path)
	-ENOENT if not found

The errors above is just an example, we wrap them with diag_set().

Until the module is explicitly unloaded the exported functions should
be alive. This is close to how kernel modules work: you can't unload
module if there are some active reference exist (for us "users" are
active functions created).

For functions we will have traditional interface

f = cbox.func.create()
f:delete()
f.__call()
f.__gc()

> Or we can remove the load reference counting. So the users will be
> forced not to call it too often and track the usages more carefully.
> Or load the functions one time in a global namespace, and unload them
> also only one time when the script is re-loaded, for example.
> 
> I advise to ask somebody who uses C stored functions in real code,
> how is it usually handled. Solution team should have experience, I
> hear they discuss C functions sometimes. Also Mons and Igor should
> now good patterns for this, because it is about Lua as well.

Will do, thanks!