From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 ; Mon, 16 Nov 2020 23:26:12 +0300 (MSK) Received: by mail-lf1-f67.google.com with SMTP id 74so26959197lfo.5 for ; Mon, 16 Nov 2020 12:26:12 -0800 (PST) Date: Mon, 16 Nov 2020 23:26:08 +0300 From: Cyrill Gorcunov Message-ID: <20201116202608.GQ2021@grain> References: <20201105151808.456573-1-gorcunov@gmail.com> <20201105151808.456573-4-gorcunov@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH v10 3/4] box/cbox: implement cbox Lua module List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tml 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 > > > > @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!