[Tarantool-patches] [RFC] cmod user api

[Tarantool-patches] [RFC] cmod user api

[Cyrill Gorcunov via Tarantool-patches]

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.

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.

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?

	Cyrill

Re: [Tarantool-patches] [RFC] cmod user api

[Vladislav Shpilevoy via Tarantool-patches]

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.

Re: [Tarantool-patches] [RFC] cmod user api

[Cyrill Gorcunov via Tarantool-patches]

On Mon, Jan 25, 2021 at 10:08:33PM +0100, Vladislav Shpilevoy wrote:
> > 
> > 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.

We've been discussing the methods with you and Mons long ago,
I even have a draft

https://gist.github.com/cyrillos/a0eaa178bfc0d22c1810c59a951421c8

and I agree that RFC should come first but you know in most cases
plain RFC is not enough, one have to implement a draft working
version which would reveal all corner cases and problems.

For example when you've been reviewing one of mine early version
you pointed about function duplicates (ie same function loaded
twise and we use load_count for this). IOW I mean that RFC is
a good pont to start but without draft working version it not
that better because implementation details might be a stopper.
I already been there - can't reveal the details but idea was
brilliant and... uncodable: the problems of implementation
ruined everything. That said I think our iterations via
implementations here is a very good approach.

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

OK. Not critical issue, we even would be able to add such support
in future by request.

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

All you said is correct the problem is -- existing code, ie a
we already have this weird reload() functionality, for functions
which are created via box.schema.func. I simply provided a hand
for this api. We can hide it and require explicit unload+load
pair instead.