Re: [Tarantool-patches] [PATCH v5 5/6] box/cbox: implement cbox Lua module

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch! I didn't review it properly yet. Just
a few (3) small comments before I will dive into details.

> diff --git a/src/box/lua/cbox.c b/src/box/lua/cbox.c
> new file mode 100644
> index 000000000..58585b44d
> --- /dev/null
> +++ b/src/box/lua/cbox.c
> @@ -0,0 +1,417 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
> + */
> +
> +#include <string.h>
> +#include <lua.h>
> +
> +#include "tt_static.h"
> +#include "assoc.h"
> +#include "diag.h"
> +#include "port.h"
> +#include "say.h"
> +
> +#include "box/error.h"
> +#include "box/func.h"

1. Why do you still depend on box/func.h? Func.h is
about stored functions with credentials and stuff. In the
previous review I proposed to move needed parts to a new
system: module_sym of whatever it is called. So it would
be module_sym.h and module_sym.c. And it would not depend
on schema at all. Currently func.h brings schema dependency
into cbox.

Maybe a good name for the new system would be module_cache.h
and module_cache.c. Like we have with coll_id_cache.h/.c.
(The structs can remain module_sym.)

> +#include "box/call.h"
> +#include "box/port.h"
> +
> +#include "trivia/util.h"
> +#include "lua/utils.h"
> +
> +/**
> + * Call function by its name from the Lua code.
> + */
> +static int
> +lcbox_func_call(struct lua_State *L)
> +{
> +	int nr_args = lua_gettop(L);
> +
> +	/*
> +	 * A function name is associated with the variable.
> +	 */
> +	lua_getfield(L, -nr_args, "name");
> +	size_t name_len;
> +	const char *name = lua_tolstring(L, -1, &name_len);
> +	struct cbox_func *cf = cbox_func_find(name, name_len);
> +	/*
> +	 * Do not pop the result early, since Lua's GC may
> +	 * drop the name string found.
> +	 */
> +	lua_pop(L, 1);
> +
> +	if (cf == NULL) {
> +		/*
> +		 * This can happen if someone changed the
> +		 * name of the function manually.
> +		 */
> +		const char *fmt = tnt_errcode_desc(ER_FUNCTION_EXISTS);
> +		diag_set(IllegalParams, fmt, name);
> +		return luaT_push_nil_and_error(L);
> +	}
> +
> +	/*
> +	 * FIXME: We should get rid of luaT_newthread but this
> +	 * requires serious modifucations. In particular
> +	 * port_lua_do_dump uses tarantool_L reference and
> +	 * coro_ref must be valid as well.
> +	 */
> +	lua_State *args_L = luaT_newthread(tarantool_L);
> +	if (args_L == NULL)
> +		return luaT_push_nil_and_error(L);
> +
> +	int coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX);
> +	lua_xmove(L, args_L, lua_gettop(L) - 1);

2. Why didn't you use lua_remove() to get rid of the unnecessary
parts of the stack?

> +
> +	struct port args;
> +	port_lua_create(&args, args_L);
> +	((struct port_lua *)&args)->ref = coro_ref;
> +
> +	struct port ret;
> +	if (module_sym_call(&cf->mod_sym, &args, &ret) != 0) {
> +		port_destroy(&args);
> +		return luaT_push_nil_and_error(L);
> +	}
> +
> +	int top = lua_gettop(L);
> +	lua_pushboolean(L, true);
> +	port_dump_lua(&ret, L, true);
> +	int cnt = lua_gettop(L) - top;
> +
> +	port_destroy(&ret);
> +	port_destroy(&args);
> +
> +	return cnt;
> +}
> +
> +/**
> + * Create a new function.
> + *
> + * This function takes a function name from the caller
> + * stack @a L and creates a new function object.
> + *
> + * Possible errors:
> + *
> + * - IllegalParams: function name is either not supplied
> + *   or not a string.
> + * - IllegalParams: a function is already created.
> + * - OutOfMemory: unable to allocate a function.
> + *
> + * @returns function object on success or {nil,error} on error,
> + * the error is set to the diagnostics area.
> + */
> +static int
> +lcbox_func_create(struct lua_State *L)
> +{
> +	const char *method = "cbox.func.create";
> +	struct cbox_func *cf = NULL;
> +
> +	if (lua_gettop(L) != 1) {
> +		const char *fmt = "Expects %s(\'name\')";
> +		diag_set(IllegalParams, fmt, method);
> +		goto out;
> +	}
> +
> +	if (lua_type(L, 1) != LUA_TSTRING) {
> +		diag_set_param_error(L, 1, method,
> +				     lua_tostring(L, 1),
> +				     "function name");
> +		goto out;
> +	}
> +
> +	size_t name_len;
> +	const char *name = lua_tolstring(L, 1, &name_len);
> +
> +	if (cbox_func_find(name, name_len) != NULL) {
> +		const char *fmt = tnt_errcode_desc(ER_FUNCTION_EXISTS);
> +		diag_set(IllegalParams, fmt, name);
> +		goto out;
> +	}
> +
> +	size_t size = sizeof(*cf) + name_len + 1;
> +	cf = malloc(size);
> +	if (cf == NULL) {
> +		diag_set(OutOfMemory, size, "malloc", "cf");
> +		goto out;
> +	}
> +
> +	cf->mod_sym.addr	= NULL;
> +	cf->mod_sym.module	= NULL;
> +	cf->mod_sym.name	= cf->name;
> +	cf->name_len		= name_len;
> +
> +	memcpy(cf->name, name, name_len);
> +	cf->name[name_len] = '\0';
> +
> +	if (cbox_func_add(cf) != 0)
> +		goto out;
> +
> +	/*
> +	 * A new variable should be callable for
> +	 * convenient use in Lua.
> +	 */
> +	lua_newtable(L);
> +
> +	lua_pushstring(L, "name");
> +	lua_pushstring(L, cf->name);
> +	lua_settable(L, -3);
> +
> +	lua_newtable(L);
> +
> +	lua_pushstring(L, "__call");
> +	lua_pushcfunction(L, lcbox_func_call);
> +	lua_settable(L, -3);
> +
> +	lua_setmetatable(L, -2);

3. What happens when the function object is deleted by Lua GC?
It seems the user can't get the function again without dropping
it.

For instance, consider this:

	box.cfg{}
	cbox = require('cbox')
	function callpush()
	    local f = cbox.func.create('function1.test_push')
	    return f({1})
	end
	callpush()
	collectgarbage('collect')
	callpush()

The second 'callpush()' will fail. And calling drop() after
each usage also looks bad - super slow because will unload the
module potentially, and won't work if some other function will
throw an exception beforehand.

To fix it I need to drop the 'test_push' function using
cbox.func.drop() right before I call create(). This looks
counter-intuitive.

So here I see 2 options how to fix it:

	- Implement a GC handler, which will drop the function, when its
	  Lua object is deleted by GC.

Or

	- Add option table to func.create, where a single option so far
	  would be {if_exists = <boolean>}. Like we do for _func space
	  in schema.lua. So users will need to call if_exists = false to
	  either load the function or get an existing ref.

Another possibility: replace create() and drop() with load() and unload(),
and add these 3 steps:

	1. Load() called on the same function name will just return the same
	  object. The object is referenced. In GC handler it is unreferenced.

	2. Unload() marks the function to be removed. If the number of loaded
	  Lua refs to this function is > 0, do nothing. If 0, delete now from
	  the hash and probably unload the entire module.

	3. Load() in GC handler checks if this is the last ref dropped. If yes,
	  remove from the hash, and probably unload the module.

Note, that even if a function was loaded, but then its ref counter becomes 0,
it is not unloaded until explicit unload() called. We really-really don't want
to load/unload the module too often, it is a super slow operation.

I personally think the second way looks better. But we can discuss. I am
not sure on 100% that it is correct.

> +	return 1;
> +
> +out:
> +	free(cf);
> +	return luaT_push_nil_and_error(L);
> +}