diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index b98f504d3269808eb83fec0b96ddce30904979a3..62fbc3a7ade6aa5155dcbacecacf96483197f02c 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1306,6 +1306,11 @@ static void
tx_fiber_init(struct session *session, uint64_t sync)
{
struct fiber *f = fiber();
+ /*
+ * There should not be any not executed on_stop triggers
+ * from a previous request executed in that fiber.
+ */
+ assert(rlist_empty(&f->on_stop));
f->storage.net.sync = sync;
/*
* We do not cleanup fiber keys at the end of each request.
@@ -1710,8 +1715,6 @@ tx_process_sql(struct cmsg *m)
const char *sql;
uint32_t len;
- tx_fiber_init(msg->connection->session, msg->header.sync);
-
if (tx_check_schema(msg->header.schema_version))
goto error;
assert(msg->header.type == IPROTO_EXECUTE);
diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
index b813c17399eba701decd12dfac0fe97792cd3319..244ef7c63b384191dc663aea8a070b6903805860 100644
--- a/src/lib/core/fiber.c
+++ b/src/lib/core/fiber.c
@@ -193,6 +193,25 @@ fiber_attr_getstacksize(struct fiber_attr *fiber_attr)
fiber_attr_default.stack_size;
}
+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");
+ /*
+ * 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.
+ */
+ assert(rlist_empty(&f->on_stop));
+}
+
static void
fiber_recycle(struct fiber *fiber);
@@ -720,8 +739,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);
/* reset pending wakeups */
rlist_del(&fiber->state);
if (! (fiber->flags & FIBER_IS_JOINABLE))
@@ -1301,8 +1319,8 @@ cord_cojoin(struct cord *cord)
void
break_ev_loop_f(struct trigger *trigger, void *event)
{
- (void) trigger;
(void) event;
+ trigger_clear(trigger);
ev_break(loop(), EVBREAK_ALL);
}
diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h
index fb168e25e2ba56576b46ce5bb0ef3829f9d3c36f..67ec79118c30c3cbac1a6d2d5708067c251dae11 100644
--- a/src/lib/core/fiber.h
+++ b/src/lib/core/fiber.h
@@ -393,7 +393,15 @@ struct fiber {
/** Triggers invoked before this fiber yields. Must not throw. */
struct rlist on_yield;
- /** Triggers invoked before this fiber stops. Must not throw. */
+ /**
+ * 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.
+ * In particular, for fibers not from a fiber pool the
+ * stop event is emitted before destruction and death.
+ * Pooled fibers receive the stop event after each
+ * request, even if they are never destroyed.
+ */
struct rlist on_stop;
/**
* The list of fibers awaiting for this fiber's timely
@@ -441,6 +449,10 @@ struct fiber {
char name[FIBER_NAME_MAX + 1];
};
+/** Invoke on_stop triggers and delete them. */
+void
+fiber_on_stop(struct fiber *f);
+
struct cord_on_exit;
/**
diff --git a/src/lib/core/fiber_pool.c b/src/lib/core/fiber_pool.c
index 77f89c9fac00539c3344cb4c72d2285eb8127b2a..d40c9bcc31aa1ffbb9b7192d05ff2ffba4684657 100644
--- a/src/lib/core/fiber_pool.c
+++ b/src/lib/core/fiber_pool.c
@@ -62,6 +62,16 @@ fiber_pool_f(va_list ap)
assert(f->caller->caller == &cord->sched);
}
cmsg_deliver(msg);
+ /*
+ * Normally fibers die after their function
+ * returns, and they call on_stop() triggers. The
+ * pool optimization interferes into that logic
+ * and a fiber doesn't die after its function
+ * returns. But on_stop triggers still should be
+ * called so that the pool wouldn't affect fiber's
+ * visible lifecycle.
+ */
+ fiber_on_stop(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 b137b27d26d5814f26b871c6bfb0d8a5b0929cfd..a4950bd81b43a410eb020769239243590b4bf27e 100644
--- a/src/lua/fiber.c
+++ b/src/lua/fiber.c
@@ -364,11 +364,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,
@@ -529,12 +524,41 @@ lbox_fiber_name(struct lua_State *L)
}
}
+/**
+ * Trigger invoked when the fiber has stopped 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 void
+lbox_fiber_on_stop(struct trigger *trigger, void *event)
+{
+ 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 = LUA_NOREF;
+ trigger_clear(trigger);
+ free(trigger);
+}
+
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_stop, NULL, (trigger_f0) free);
+ trigger_add(&f->on_stop, 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 0000000000000000000000000000000000000000..cec1aba626e2c66d32e95180a27f96bcefd9c0ab
--- /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 fiber 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 0000000000000000000000000000000000000000..7a582d40e787bfc477af6dadd99728d43d83e262
--- /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 fiber 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')