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

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

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
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 are on_cleanup triggers, and can be called multiple times
during fiber's lifetime. After every request done by that fiber.

Closes #4662
Closes #3462
---
 src/box/session.cc                           | 11 ++-
 src/box/session.h                            |  7 +-
 src/box/txn.c                                | 13 +--
 src/box/txn.h                                |  7 +-
 src/lib/core/fiber.c                         | 23 +++--
 src/lib/core/fiber.h                         | 13 ++-
 src/lib/core/fiber_pool.c                    |  6 ++
 src/lua/fiber.c                              | 35 ++++++--
 test/app/gh-4662-fiber-storage-leak.result   | 88 ++++++++++++++++++++
 test/app/gh-4662-fiber-storage-leak.test.lua | 43 ++++++++++
 10 files changed, 217 insertions(+), 29 deletions(-)
 create mode 100644 test/app/gh-4662-fiber-storage-leak.result
 create mode 100644 test/app/gh-4662-fiber-storage-leak.test.lua

diff --git a/src/box/session.cc b/src/box/session.cc
index 461d1cf25..a08043986 100644
--- a/src/box/session.cc
+++ b/src/box/session.cc
@@ -81,7 +81,7 @@ sid_max()
 }
 
 static int
-session_on_stop(struct trigger *trigger, void * /* event */)
+session_on_fiber_cleanup(struct trigger *trigger, void * /* event */)
 {
 	/*
 	 * Remove on_stop trigger from the fiber, otherwise the
@@ -167,11 +167,10 @@ session_create_on_demand()
 	struct session *s = session_create(SESSION_TYPE_BACKGROUND);
 	if (s == NULL)
 		return NULL;
-	s->fiber_on_stop = {
-		RLIST_LINK_INITIALIZER, session_on_stop, NULL, NULL
-	};
-	/* Add a trigger to destroy session on fiber stop */
-	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);
 	credentials_reset(&s->credentials, admin_user);
 	fiber_set_session(fiber(), s);
 	fiber_set_user(fiber(), &s->credentials);
diff --git a/src/box/session.h b/src/box/session.h
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;
 };
 
 struct session_vtab {
diff --git a/src/box/txn.c b/src/box/txn.c
index 963ec8eeb..7d549a6a9 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -41,7 +41,7 @@ double too_long_threshold;
 static struct stailq txn_cache = {NULL, &txn_cache.first};
 
 static int
-txn_on_stop(struct trigger *trigger, void *event);
+txn_on_fiber_cleanup(struct trigger *trigger, void *event);
 
 static int
 txn_on_yield(struct trigger *trigger, void *event);
@@ -226,8 +226,9 @@ txn_begin()
 	txn->fiber = NULL;
 	fiber_set_txn(fiber(), txn);
 	/* fiber_on_yield is initialized by engine on demand */
-	trigger_create(&txn->fiber_on_stop, txn_on_stop, NULL, NULL);
-	trigger_add(&fiber()->on_stop, &txn->fiber_on_stop);
+	trigger_create(&txn->fiber_on_cleanup, txn_on_fiber_cleanup,
+		       NULL, NULL);
+	trigger_add(&fiber()->on_cleanup, &txn->fiber_on_cleanup);
 	/*
 	 * By default all transactions may yield.
 	 * It's a responsibility of an engine to disable yields
@@ -550,7 +551,7 @@ txn_prepare(struct txn *txn)
 		if (engine_prepare(txn->engine, txn) != 0)
 			return -1;
 	}
-	trigger_clear(&txn->fiber_on_stop);
+	trigger_clear(&txn->fiber_on_cleanup);
 	if (!txn_has_flag(txn, TXN_CAN_YIELD))
 		trigger_clear(&txn->fiber_on_yield);
 	return 0;
@@ -618,7 +619,7 @@ void
 txn_rollback(struct txn *txn)
 {
 	assert(txn == in_txn());
-	trigger_clear(&txn->fiber_on_stop);
+	trigger_clear(&txn->fiber_on_cleanup);
 	if (!txn_has_flag(txn, TXN_CAN_YIELD))
 		trigger_clear(&txn->fiber_on_yield);
 	txn->signature = -1;
@@ -840,7 +841,7 @@ txn_savepoint_release(struct txn_savepoint *svp)
 }
 
 static int
-txn_on_stop(struct trigger *trigger, void *event)
+txn_on_fiber_cleanup(struct trigger *trigger, void *event)
 {
 	(void) trigger;
 	(void) event;
diff --git a/src/box/txn.h b/src/box/txn.h
index da12feebf..755c464f0 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -207,10 +207,11 @@ struct txn {
 	 */
 	struct trigger fiber_on_yield;
 	/**
-	 * Trigger on fiber stop, to rollback transaction
-	 * in case a fiber stops (all engines).
+	 * Trigger on fiber cleanup, to rollback transaction
+	 * in case a fiber is getting reset/recycled/destroyed
+	 * (all engines).
 	 */
-	struct trigger fiber_on_stop;
+	struct trigger fiber_on_cleanup;
 	/** Commit and rollback triggers. */
 	struct rlist on_commit, on_rollback;
 	/**
diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
index 00ae8cded..db8fdd5fe 100644
--- a/src/lib/core/fiber.c
+++ b/src/lib/core/fiber.c
@@ -325,6 +325,20 @@ fiber_attr_getstacksize(struct fiber_attr *fiber_attr)
 				    fiber_attr_default.stack_size;
 }
 
+void
+fiber_cleanup(struct fiber *f)
+{
+	if (trigger_run(&f->on_cleanup, f) != 0)
+		panic("Fiber cleanup can't fail");
+	/*
+	 * Double call of cleanup routines is a bad idea. It is
+	 * like calling a destructor 2 times. On_cleanup can be
+	 * called only once until the fiber is reused again, and
+	 * new triggers are set.
+	 */
+	trigger_destroy(&f->on_cleanup);
+}
+
 static void
 fiber_recycle(struct fiber *fiber);
 
@@ -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);
 		/* reset pending wakeups */
 		rlist_del(&fiber->state);
 		if (! (fiber->flags & FIBER_IS_JOINABLE))
@@ -1161,7 +1174,7 @@ fiber_destroy(struct cord *cord, struct fiber *f)
 	assert(f != &cord->sched);
 
 	trigger_destroy(&f->on_yield);
-	trigger_destroy(&f->on_stop);
+	trigger_destroy(&f->on_cleanup);
 	rlist_del(&f->state);
 	rlist_del(&f->link);
 	region_destroy(&f->gc);
@@ -1555,7 +1568,7 @@ cord_costart_thread_func(void *arg)
 	 * Got to be in a trigger, to break the loop even
 	 * in case of an exception.
 	 */
-	trigger_add(&f->on_stop, &break_ev_loop);
+	trigger_add(&f->on_cleanup, &break_ev_loop);
 	fiber_set_joinable(f, true);
 	fiber_start(f, ctx.arg);
 	if (!fiber_is_dead(f)) {
diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h
index c5b975513..4e21b9999 100644
--- a/src/lib/core/fiber.h
+++ b/src/lib/core/fiber.h
@@ -458,8 +458,13 @@ 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.
+	 */
+	struct rlist on_cleanup;
 	/**
 	 * The list of fibers awaiting for this fiber's timely
 	 * (or untimely) death.
@@ -506,6 +511,10 @@ struct fiber {
 	char name[FIBER_NAME_MAX + 1];
 };
 
+/** Invoke cleanup triggers and delete them. */
+void
+fiber_cleanup(struct fiber *f);
+
 struct cord_on_exit;
 
 /**
diff --git a/src/lib/core/fiber_pool.c b/src/lib/core/fiber_pool.c
index 77f89c9fa..31f366fac 100644
--- a/src/lib/core/fiber_pool.c
+++ b/src/lib/core/fiber_pool.c
@@ -62,6 +62,12 @@ restart:
 			assert(f->caller->caller == &cord->sched);
 		}
 		cmsg_deliver(msg);
+		/*
+		 * Different messages = different requests. They
+		 * should not affect each other. This is why
+		 * cleanup is done here.
+		 */
+		fiber_cleanup(f);
 	}
 	/** Put the current fiber into a fiber cache. */
 	if (!fiber_is_cancelled(fiber()) && (msg != NULL ||
diff --git a/src/lua/fiber.c b/src/lua/fiber.c
index a7b75f9bf..cba625082 100644
--- a/src/lua/fiber.c
+++ b/src/lua/fiber.c
@@ -441,11 +441,6 @@ lua_fiber_run_f(MAYBE_UNUSED va_list ap)
 	int coro_ref = lua_tointeger(L, -1);
 	lua_pop(L, 1);
 	result = luaT_call(L, lua_gettop(L) - 1, LUA_MULTRET);
-
-	/* Destroy local storage */
-	int storage_ref = f->storage.lua.ref;
-	if (storage_ref > 0)
-		luaL_unref(tarantool_L, LUA_REGISTRYINDEX, storage_ref);
 	/*
 	 * If fiber is not joinable
 	 * We can unref child stack here,
@@ -606,12 +601,42 @@ 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)
+{
+	(void) trigger;
+	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;
+	return 0;
+}
+
 static int
 lbox_fiber_storage(struct lua_State *L)
 {
 	struct fiber *f = lbox_checkfiber(L, 1);
 	int storage_ref = f->storage.lua.ref;
 	if (storage_ref <= 0) {
+		struct trigger *t = (struct trigger *)
+			malloc(sizeof(*t));
+		if (t == NULL) {
+			diag_set(OutOfMemory, sizeof(*t), "malloc", "t");
+			return luaT_error(L);
+		}
+		trigger_create(t, lbox_fiber_on_cleanup, NULL,
+			       (trigger_f0) free);
+		trigger_add(&f->on_cleanup, t);
 		lua_newtable(L); /* create local storage on demand */
 		storage_ref = luaL_ref(L, LUA_REGISTRYINDEX);
 		f->storage.lua.ref = storage_ref;
diff --git a/test/app/gh-4662-fiber-storage-leak.result b/test/app/gh-4662-fiber-storage-leak.result
new file mode 100644
index 000000000..4ade017a4
--- /dev/null
+++ b/test/app/gh-4662-fiber-storage-leak.result
@@ -0,0 +1,88 @@
+-- test-run result file version 2
+fiber = require('fiber')
+ | ---
+ | ...
+netbox = require('net.box')
+ | ---
+ | ...
+
+--
+-- gh-4662: 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.
+-- The test checks if fiber.storage is really deleted, and is not
+-- shared between requests.
+--
+
+box.schema.user.grant('guest', 'execute', 'universe')
+ | ---
+ | ...
+storage = nil
+ | ---
+ | ...
+i = 0
+ | ---
+ | ...
+weak_table = setmetatable({}, {__mode = 'v'})
+ | ---
+ | ...
+object = {'object'}
+ | ---
+ | ...
+weak_table.object = object
+ | ---
+ | ...
+function ref_object_in_fiber()                  \
+    storage = fiber.self().storage              \
+    assert(next(storage) == nil)                \
+    i = i + 1                                   \
+    fiber.self().storage.key = i                \
+    fiber.self().storage.object = object        \
+end
+ | ---
+ | ...
+
+c = netbox.connect(box.cfg.listen)
+ | ---
+ | ...
+c:call('ref_object_in_fiber') c:call('ref_object_in_fiber')
+ | ---
+ | ...
+storage
+ | ---
+ | - key: 2
+ |   object:
+ |   - object
+ | ...
+i
+ | ---
+ | - 2
+ | ...
+object = nil
+ | ---
+ | ...
+storage = nil
+ | ---
+ | ...
+collectgarbage('collect')
+ | ---
+ | - 0
+ | ...
+-- The weak table should be empty, because the only two hard
+-- references were in the fibers used to serve
+-- ref_object_in_fiber() requests. And their storages should be
+-- cleaned up.
+weak_table
+ | ---
+ | - []
+ | ...
+
+c:close()
+ | ---
+ | ...
+box.schema.user.revoke('guest', 'execute', 'universe')
+ | ---
+ | ...
diff --git a/test/app/gh-4662-fiber-storage-leak.test.lua b/test/app/gh-4662-fiber-storage-leak.test.lua
new file mode 100644
index 000000000..25acf5679
--- /dev/null
+++ b/test/app/gh-4662-fiber-storage-leak.test.lua
@@ -0,0 +1,43 @@
+fiber = require('fiber')
+netbox = require('net.box')
+
+--
+-- gh-4662: 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.
+-- The test checks if fiber.storage is really deleted, and is not
+-- shared between requests.
+--
+
+box.schema.user.grant('guest', 'execute', 'universe')
+storage = nil
+i = 0
+weak_table = setmetatable({}, {__mode = 'v'})
+object = {'object'}
+weak_table.object = object
+function ref_object_in_fiber()                  \
+    storage = fiber.self().storage              \
+    assert(next(storage) == nil)                \
+    i = i + 1                                   \
+    fiber.self().storage.key = i                \
+    fiber.self().storage.object = object        \
+end
+
+c = netbox.connect(box.cfg.listen)
+c:call('ref_object_in_fiber') c:call('ref_object_in_fiber')
+storage
+i
+object = nil
+storage = nil
+collectgarbage('collect')
+-- The weak table should be empty, because the only two hard
+-- references were in the fibers used to serve
+-- ref_object_in_fiber() requests. And their storages should be
+-- cleaned up.
+weak_table
+
+c:close()
+box.schema.user.revoke('guest', 'execute', 'universe')
-- 
2.21.0 (Apple Git-122.2)