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

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

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/12/08 23:47]:
>  static int
> -session_on_stop(struct trigger *trigger, void * /* event */)
> +session_on_fiber_cleanup(struct trigger *trigger, void * /* event */)

Please come up with a better name, and provide a comment.

> -	trigger_add(&fiber()->on_stop, &s->fiber_on_stop);
> +	trigger_create(&s->fiber_on_cleanup, session_on_fiber_cleanup,
> +		       NULL, NULL);
> +	/* Add a trigger to destroy session on fiber cleanup. */
> +	trigger_add(&fiber()->on_cleanup, &s->fiber_on_cleanup);

I don't understand why these two concerns are intermixed here.
Why is this code part of session now? 

I think if you change the semantics of the trigger, you should 
move setting/resetting it to the place which controls the relevant
object and its life cycle. Specifically, if you change
fiber.storage semantics to be per-request *or* per Lua fiber,
you should move the trigger to lua fiber create/destroy +
iproto request processing start/end. Instead you move it
to the core library, which is oblivious of these events. 
The way the core is implemented may change, and whoever is
changing it will have to be aware of the legacy behaviour you make
it depend on.

> index eff3d7a67..710e4919b 100644
> --- a/src/box/session.h
> +++ b/src/box/session.h
> @@ -103,8 +103,11 @@ struct session {
>  	union session_meta meta;
>  	/** Session user id and global grants */
>  	struct credentials credentials;
> -	/** Trigger for fiber on_stop to cleanup created on-demand session */
> -	struct trigger fiber_on_stop;
> +	/**
> +	 * Trigger for fiber on_cleanup to cleanup created
> +	 * on-demand session.
> +	 */
> +	struct trigger fiber_on_cleanup;
...

> -	trigger_clear(&txn->fiber_on_stop);
> +	trigger_clear(&txn->fiber_on_cleanup);

Please see the above comment. Fiber cleanup trigger, as defined
now, does not have anything to do with the session, or fiber, or
transaction.
It belongs to the request processing layer and should be
set/cleared from it.

> -	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.
> +	 */
> +	struct rlist on_cleanup;

Please make the comment describing the timing when this
trigger is called less broad and more accurate, calling out
the specific places when the trigger is called, and call
the trigger accordingly.
on_request_or_fiber_end?

> +/** Invoke cleanup triggers and delete them. */
> +void
> +fiber_cleanup(struct fiber *f);

The call for a better name for the event and the accompanying 
action applies to all of the new api
methods this patch introduces.

>  		cmsg_deliver(msg);
> +		/*
> +		 * Different messages = different requests. They
> +		 * should not affect each other. This is why
> +		 * cleanup is done here.
> +		 */
> +		fiber_cleanup(f);

Ugh. I think this is a layering violation, and the cleanup
call should be somewhere inside cmsg_deliver for the relevant
messages. The rlist with cleanup triggers is better stored in
a fiber key.

-- 
Konstantin Osipov, Moscow, Russia