[Tarantool-patches] [PATCH v2 2/3] fiber: destroy fiber.storage created by iproto
Konstantin Osipov
kostja.osipov at gmail.com
Fri Jan 17 10:45:53 MSK 2020
* Vladislav Shpilevoy <v.shpilevoy at 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
More information about the Tarantool-patches
mailing list