Re: [Tarantool-patches] [PATCH 2/2] fiber: destroy fiber.storage created by iproto

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the review!

>> diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
>> index 00ae8cded..d89c640aa 100644
>> --- a/src/lib/core/fiber.c
>> +++ b/src/lib/core/fiber.c
>> @@ -787,7 +801,7 @@ static void
>>  fiber_reset(struct fiber *fiber)
>>  {
>>  	rlist_create(&fiber->on_yield);
>> -	rlist_create(&fiber->on_stop);
>> +	rlist_create(&fiber->on_cleanup);
>>  	fiber->flags = FIBER_DEFAULT_FLAGS;
>>  #if ENABLE_FIBER_TOP
>>  	clock_stat_reset(&fiber->clock_stat);
>> @@ -856,8 +870,7 @@ fiber_loop(MAYBE_UNUSED void *data)
>>  		       assert(f != fiber);
>>  		       fiber_wakeup(f);
>>  	        }
>> -		if (! rlist_empty(&fiber->on_stop))
>> -			trigger_run(&fiber->on_stop, fiber);
>> +	        fiber_cleanup(fiber);
> 
> I see you dropped the if above and fiber_cleanup doesn't have any
> corresponding within. Is this removal an intentional one?

Yes. This is because trigger_run works fine with empty
rlist, so this check was useless.

> 
> Minor: There is a mess with whitespace above. Feel free to ignore the
> remark, since indentation was broken before your changes. I blamed
> several lines and it looks more like a ridiculous code style, and not a
> problem with editor. Thereby you just implicitly continue the mix.

Wow, good catch! I didn't notice it in git diff nor in the
editor. Fixed.

> 
>>  		/* reset pending wakeups */
>>  		rlist_del(&fiber->state);
>>  		if (! (fiber->flags & FIBER_IS_JOINABLE))
>> diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h
>> index c5b975513..21fff8f88 100644
>> --- a/src/lib/core/fiber.h
>> +++ b/src/lib/core/fiber.h
>> @@ -458,8 +458,17 @@ struct fiber {
>>  
>>  	/** Triggers invoked before this fiber yields. Must not throw. */
>>  	struct rlist on_yield;
>> -	/** Triggers invoked before this fiber stops.  Must not throw. */
>> -	struct rlist on_stop;
>> +	/**
>> +	 * Triggers invoked before this fiber is
>> +	 * stopped/reset/recycled/destroyed/reused. In other
>> +	 * words, each time when the fiber has finished execution
>> +	 * of a request.
>> +	 * In particular, for fibers not from fiber pool the
>> +	 * cleanup is done before destroy and death.
>> +	 * Pooled fibers are cleaned up after each request, even
>> +	 * if they are never destroyed.
>> +	 */
>> +	struct rlist on_cleanup;
> 
> Minor: both trigger lists above has the "Must not throw" note. Does the
> introduced list respect this rule? Please, adjust the comment if yes.

Nope. These 'must not throw' are outdated. Our triggers does not
throw errors anymore by design. So I deleted it from the updated
comment.

>>  	/**
>>  	 * The list of fibers awaiting for this fiber's timely
>>  	 * (or untimely) death.
>> diff --git a/src/lua/fiber.c b/src/lua/fiber.c
>> index a7b75f9bf..4d2828f1c 100644
>> --- a/src/lua/fiber.c
>> +++ b/src/lua/fiber.c
>> @@ -606,12 +601,43 @@ lbox_fiber_name(struct lua_State *L)
>>  	}
>>  }
>>  
>> +/**
>> + * Trigger invoked when the fiber has finished execution of its
>> + * current request. Only purpose - delete storage.lua.ref keeping
>> + * a reference of Lua fiber.storage object. Unlike Lua stack,
>> + * Lua fiber storage may be created not only for fibers born from
>> + * Lua land. For example, an IProto request may execute a Lua
>> + * function, which can create the storage. Trigger guarantees,
>> + * that even for non-Lua fibers the Lua storage is destroyed.
>> + */
>> +static int
>> +lbox_fiber_on_cleanup(struct trigger *trigger, void *event)
>> +{
>> +	struct fiber *f = (struct fiber *) event;
>> +	int storage_ref = f->storage.lua.ref;
>> +	assert(storage_ref > 0);
>> +	luaL_unref(tarantool_L, LUA_REGISTRYINDEX, storage_ref);
>> +	f->storage.lua.ref = 0;
> 
> Though 0 value is specific "ref" (a table slot, where the LRU ref is
> stored) and this value cannot be yield from luaL_ref. Please, consider
> using LUA_NOREF value for such cases, since it's being provided by Lua
> and lua_unref does nothing for it, as well as for LUA_REFNIL.

Thanks, didn't know about these constants.

===================================================================
 	assert(storage_ref > 0);
 	luaL_unref(tarantool_L, LUA_REGISTRYINDEX, storage_ref);
-	f->storage.lua.ref = 0;
+	f->storage.lua.ref = LUA_NOREF;
 	trigger_clear(trigger);
 	free(trigger);
===================================================================