Re: [Tarantool-patches] [PATCH v2 3/3] lua: expose temporary Lua state for iproto calls

[Igor Munkin <imun@tarantool.org>]

Sasha,

Thanks for the comments, LGTM!

On 16.07.20, Alexander Turenko wrote:

<snipped>

> > > diff --git a/src/box/lua/call.c b/src/box/lua/call.c
> > > index 6588ec2fa..ccdef6662 100644
> > > --- a/src/box/lua/call.c
> > > +++ b/src/box/lua/call.c
> > > @@ -537,12 +537,39 @@ box_process_lua(lua_CFunction handler, struct execute_lua_ctx *ctx,
> > >  	port_lua_create(ret, L);
> > >  	((struct port_lua *) ret)->ref = coro_ref;
> > >  
> > > +	/*
> > > +	 * A code that need a temporary fiber-local Lua state may
> > > +	 * save some time and resources for creating a new state
> > > +	 * and use this one.
> > > +	 */
> > 
> > Could you please provide an example for the fiber calling this function
> > with non-NULL fiber-local Lua state?
> 
> Sure.
> 
>  | tarantool> box.cfg{}
>  | tarantool> echo = function(...) return ... end
>  | tarantool> box.schema.func.create('echo')
>  | tarantool> box.schema.func.call('echo', {1, 2, 3})

Well, I expected this one, thanks.

> 
> I added the assert and verified it just in case:
> 
>  | diff --git a/src/box/lua/call.c b/src/box/lua/call.c
>  | index 0315e720c..0221ffd2d 100644
>  | --- a/src/box/lua/call.c
>  | +++ b/src/box/lua/call.c
>  | @@ -561,6 +561,7 @@ box_process_lua(enum handlers handler, struct execute_lua_ctx *ctx,
>  |          * and use this one.
>  |          */
>  |         bool has_lua_stack = fiber()->storage.lua.stack != NULL;
>  | +       assert(fiber()->storage.lua.stack == NULL);
>  |         if (!has_lua_stack)
>  |                 fiber()->storage.lua.stack = L;
> 
> The assert fails after the steps above.
> 
> (But even if it would not be possible, I would write the code this way
> to don't lean on not-so-obvious details that may be changed in a
> future.)
> 
> > Are those conditions below are strictly required by the current
> > implementation?
> 
> When the fiber-local Lua state is present it should not be changed or
> zapped by the function. I don't know whether it would lead to some
> negative behaviour changes, but it would be at least counter-intuitive.
> There is the comment on the topic (I left it cited below).

My last sentence relates to the check whether fiber-local Lua state is
present. Long story short, I see no reasons to omit this field
initialization prior to <box_process_lua> call. Feel free to consider
this one as a side note with no changes required.

> 
> > > +	 * But: keep the stack if it was present before the call,
> > > +	 * because it would be counter-intuitive if the existing
> > > +	 * state pointer would be zapped after this function call.
> > > +	 */
> > > +	if (!has_lua_stack)
> > > +		fiber()->storage.lua.stack = NULL;
> 
> BTW, 'present' is here again. Don't know what form is better here, but
> left it as is now.

I'm OK with this wording after your clarifications.

-- 
Best regards,
IM