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

[Konstantin Osipov <kostja.osipov@gmail.com>]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [20/01/17 00:57]:
> Fiber.storage was not deleted when created in a fiber started from
> the thread pool used by IProto requests. The problem was that
> fiber.storage was created and deleted in Lua land only, assuming
> that only Lua-born fibers could have it. But in fact any fiber can
> create a Lua storage. Including the ones used to serve IProto
> requests.
> 
> Not deletion of the storage led to a possibility of meeting a
> non-empty fiber.storage in the beginning of an iproto request, and
> to not deletion of the memory caught by the storage until its
> explicit nullification.
> 
> Now the storage destructor works for any fiber, which managed to
> create the storage. The destructor unrefs and nullifies the
> storage.
> 
> For destructor purposes the fiber.on_stop triggers were reworked.
> Now they can be called multiple times during fiber's lifetime.
> After every request done by that fiber.
> 
> Closes #4662
> Closes #3462

I like this version of the patch much more than the previous one.
It's way more clear. Instead of a generic name, you made  a good
comment for fiber::on_stop.

> +/**
> + * Stop the current fiber after a request is executed to make it
> + * possible to reuse the fiber for a next request. On_stop
> + * triggers remove all request-specific data from there.
> + */
> +static inline void
> +tx_fiber_on_stop()
> +{
> +	fiber_on_stop(fiber());
> +}
> +
>  static void
>  tx_process_disconnect(struct cmsg *m)
>  {
> @@ -1335,6 +1351,7 @@ tx_process_disconnect(struct cmsg *m)
>  		if (! rlist_empty(&session_on_disconnect)) {
>  			tx_fiber_init(con->session, 0);
>  			session_run_on_disconnect_triggers(con->session);
> +			tx_fiber_on_stop();
>  		}
>  	}

Why did you have to add so many invocation points to
fiber_on_stop() rather than simply adding fiber_on_stop invocation to
fiber_pool.c?


Maybe we discussed this, but it's been 3 weeks ago and I lost the
context/rationale.

> +void
> +fiber_on_stop(struct fiber *f)
> +{
> +	if (trigger_run(&f->on_stop, f) != 0)
> +		panic("On_stop triggers can't fail");
> +	/*
> +	 * All on_stop triggers are supposed to remove themselves.
> +	 * So as no to waste time on that here, and to make them
> +	 * all work uniformly.
> +	 */

Nice.

> +	assert(rlist_empty(&f->on_stop));
> +}
> +


>  static void
>  fiber_recycle(struct fiber *fiber);
>  
> @@ -856,8 +869,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_on_stop(fiber);

This was an attempt to optimize a non-inline function call
for the most common case.

I would move this !rlist_empty check to fiber_on_stop and add a
comment why we explicitly check for the list first.


-- 
Konstantin Osipov, Moscow, Russia