From: Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Cyrill Gorcunov <gorcunov@gmail.com>, TML <tarantool-patches@dev.tarantool.org> Cc: Mons Anderson <v.perepelitsa@corp.mail.ru> Subject: Re: [Tarantool-patches] [RFC] cmod user api Date: Mon, 25 Jan 2021 22:08:33 +0100 [thread overview] Message-ID: <9beb6862-b21a-a054-e66d-b744152c1be0@tarantool.org> (raw) In-Reply-To: <20210125203550.GH2174@grain> Hi! On 25.01.2021 21:35, Cyrill Gorcunov wrote: > Hi guys! Recently I've sent a series which implements that > named "cmod" module which allows to load and run stored C > procedures similar to 'box.func' except they allows to run > such functions even on nodes in read-only mode. > > The overall picture of api was not clear from changelog and > here I provide a short description to gather comments. It is clear from the changelog. The problem was that the API was *implemented* before getting approves on it. For big public features like this it usually leads to huge waste of time and disappointment of the author when patches get rejected due to implementing something in a wrong way. Which is not the case for this version I hope. For me all looks sane in the API except reload. > Entry point of any function comes from "module" instance. > To load a module one have to > > module = require('cmod').load('path-to-module') > > Note the path to the module should be shipped without .so > extension (or dylib for macos). I suspect we should support > both variants with and without file extension. Nevermind, it is platform-dependent. I think it is fine just like it works in _func now - file name without an extension. > Once module is loaded we can associate a function from > this module with some Lua variable. > > foo = module:load('name-of-function') > > Then we can execute it > > res = foo(arguments, ...) > > Both function and module supports explicit unloading via > unload method. > > foo:unload() > module:unload() > > When function/module is unloaded any attempt to use this > variable will produce an error. If they are not explicitly > unloaded then GC will reap them. > > Module Lua object provides :reload() method which re-reads > the shared library and updates associated functions such than > any new call will be executed via new code. If there are some > already executing functions which are in yield state then such > functions gonna finish execution of old code first and only > next calls will be pacing the new code. > > Does such api looks sane or there some other ideas? I will copy-paste here what I said about reloading the existing function objects under the hood. Reason for not changing the existing function objects was that the reload is supposed to happen when the whole application reloads. It means there can be some fibers running the old code for a while after reload until they notice that they must be restarted. If you do reload like you did in this patch, the old fibers will get new functions immediately right under their feet, and this may result into unexpected behaviour for code in these fibers. For instance, you did this code: local func = mod:load('do_replace_and_yield') for i, obj in pairs(objs) do func(obj) end func:unload() Assume 'do_replace_and_yield' yields inside. Also assume that the loop is started in a separate fiber, and func is called first time. It yields, and now the whole Lua module, having this code, is reloaded. This old code still runs in the fiber, created in the beginning before reload. During Lua application reload the shared module was also reloaded, and now 'do_replace_and_yield' expects two arguments instead of 1. So on the next iteration of the loop 'func' will fail. The user didn't do anything with 'func' object, but it was changed inside, and the user couldn't do anything about it. Calling 'load()' on each iteration is not an option if the loop iterates thousands of times and more. All becomes even worse if 'do_replace_and_yield' does accept the arguments, but its behaviour is changed. For instance, its result format. Another way to get the desired behaviour - use unload() + load() - I assume in this case the old function objects will remain unchanged, right? But do we really need such 'reload' behaviour then? I suggest at least to ask Mons what to do with this.
next prev parent reply other threads:[~2021-01-25 21:08 UTC|newest] Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-01-25 20:35 Cyrill Gorcunov via Tarantool-patches 2021-01-25 21:08 ` Vladislav Shpilevoy via Tarantool-patches [this message] 2021-01-25 21:56 ` Cyrill Gorcunov via Tarantool-patches
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=9beb6862-b21a-a054-e66d-b744152c1be0@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=gorcunov@gmail.com \ --cc=v.perepelitsa@corp.mail.ru \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [RFC] cmod user api' \ /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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox