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

[Vladislav Shpilevoy]

Thanks for the patch!

See 3 comments below.

On 05.11.2020 16:18, Cyrill Gorcunov wrote:
> Currently to run "C" function from some external module
> one have to register it first in "_func" system space. This
> is a problem if node is in read-only mode (replica).
> 
> 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.

> Signed-off-by: Cyrill Gorcunov <gorcunov at 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.

> 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.
> 
> Module functions
> ================
> 
> `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.

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.