Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: Cyrill Gorcunov <gorcunov@gmail.com>,
	tml <tarantool-patches@dev.tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH v10 3/4] box/cbox: implement cbox Lua module
Date: Thu, 12 Nov 2020 23:53:45 +0100
Message-ID: <e186c454-6765-4776-6433-f3f791ff4c27@tarantool.org> (raw)
In-Reply-To: <20201105151808.456573-4-gorcunov@gmail.com>

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@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.

  reply	other threads:[~2020-11-12 22:53 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-05 15:18 [Tarantool-patches] [PATCH v10 0/4] box/cbox: implement cfunc " Cyrill Gorcunov
2020-11-05 15:18 ` [Tarantool-patches] [PATCH v10 1/4] box/func: factor out c function entry structure Cyrill Gorcunov
2020-11-12 22:53   ` Vladislav Shpilevoy
2020-11-13 17:56     ` Cyrill Gorcunov
2020-11-05 15:18 ` [Tarantool-patches] [PATCH v10 2/4] module_cache: move module handling into own subsystem Cyrill Gorcunov
2020-11-12 22:54   ` Vladislav Shpilevoy
2020-11-16  9:54     ` Cyrill Gorcunov
2020-11-16 14:41     ` Cyrill Gorcunov
2020-11-05 15:18 ` [Tarantool-patches] [PATCH v10 3/4] box/cbox: implement cbox Lua module Cyrill Gorcunov
2020-11-12 22:53   ` Vladislav Shpilevoy [this message]
2020-11-16 20:26     ` Cyrill Gorcunov
2020-11-05 15:18 ` [Tarantool-patches] [PATCH v10 4/4] test: box/cfunc -- add simple module test Cyrill Gorcunov
2020-11-12 22:53 ` [Tarantool-patches] [PATCH v10 0/4] box/cbox: implement cfunc Lua module Vladislav Shpilevoy
2020-11-13 17:54   ` Cyrill Gorcunov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e186c454-6765-4776-6433-f3f791ff4c27@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=gorcunov@gmail.com \
    --cc=tarantool-patches@dev.tarantool.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git