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/20 09:59]:
> > 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?
> 
> We already discussed that in the previous patch version. We decided
> to move cleanup to iproto.cc, because it depends on when a request
> ends. Fiber pool knows nothing about requests. Iproto.cc is request
> processing layer, and this is the right place for request data
> destruction.

True, but since you abstracted out the destruction via an opaque
trigger, why not move the invocation of the trigger to fiber pool? 
fiber pool has most knowledge about fiber life cycle, so it seems
natural to invoke the triggers in it - it will tie the *timing* to
fiber pool, but not what's going on inside the trigger.

Thoughts?
> > I would move this !rlist_empty check to fiber_on_stop and add a
> > comment why we explicitly check for the list first.
> 
> I doubt it really helps, but ok.
> 
> ================================================================================
> 
> diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
> index 634b3d1b0..354749549 100644
> --- a/src/lib/core/fiber.c
> +++ b/src/lib/core/fiber.c
> @@ -328,6 +328,12 @@ fiber_attr_getstacksize(struct fiber_attr *fiber_attr)
>  void
>  fiber_on_stop(struct fiber *f)
>  {
> +	/*
> +	 * The most common case is when the list is empty. Do an
> +	 * inlined check before calling trigger_run().
> +	 */
> +	if (rlist_empty(&f->on_stop))
> +		return;
>  	if (trigger_run(&f->on_stop, f) != 0)
>  		panic("On_stop triggers can't fail");
>  	/*

thanks!

-- 
Konstantin Osipov, Moscow, Russia