From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id EE50B469719 for ; Fri, 13 Nov 2020 01:53:46 +0300 (MSK) References: <20201105151808.456573-1-gorcunov@gmail.com> <20201105151808.456573-4-gorcunov@gmail.com> From: Vladislav Shpilevoy Message-ID: Date: Thu, 12 Nov 2020 23:53:45 +0100 MIME-Version: 1.0 In-Reply-To: <20201105151808.456573-4-gorcunov@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: Cyrill Gorcunov , tml 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 > > @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.