From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <gorcunov@gmail.com>
Received: from mail-lf1-f67.google.com (mail-lf1-f67.google.com
[209.85.167.67])
(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits))
(No client certificate requested)
by dev.tarantool.org (Postfix) with ESMTPS id 893CE469719
for <tarantool-patches@dev.tarantool.org>;
Mon, 16 Nov 2020 23:26:12 +0300 (MSK)
Received: by mail-lf1-f67.google.com with SMTP id 74so26959197lfo.5
for <tarantool-patches@dev.tarantool.org>;
Mon, 16 Nov 2020 12:26:12 -0800 (PST)
Date: Mon, 16 Nov 2020 23:26:08 +0300
From: Cyrill Gorcunov <gorcunov@gmail.com>
Message-ID: <20201116202608.GQ2021@grain>
References: <20201105151808.456573-1-gorcunov@gmail.com>
<20201105151808.456573-4-gorcunov@gmail.com>
<e186c454-6765-4776-6433-f3f791ff4c27@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <e186c454-6765-4776-6433-f3f791ff4c27@tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH v10 3/4] box/cbox: implement cbox
Lua module
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>,
<mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>,
<mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tml <tarantool-patches@dev.tarantool.org>
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!