[Tarantool-patches] [PATCH 1/1] txn: destroy commit and rollback triggers
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Sat Apr 24 03:29:19 MSK 2021
They were not deleted ever. Worked fine for DDL, for which they
were introduced on the first place, because these triggers are on
the region memory.
But didn't work when the triggers became available in the public
API, because these are allocated on the heap. As a result, all the
box.on_commit() and box.on_rollback() triggers leaked.
The patch ensures all the on_commit/on_rollback triggers are
destroyed. In order to mitigate the performance impact the
internal triggers now clear themselves so as it wouldn't be
necessary to walk their list in txn.c again.
Another hack not to touch these triggers when possible makes their
destruction happen only in txn_run_commit_triggers() and
txn_run_rollback_triggers(). These are not called when a
transaction does not have any triggers, and therefore these
destroys don't cost anything. On the contrary with if they would
be destroyed in txn_free() which is called always.
In short: the patch does not affect the common path when the
triggers are not used.
Another option was to force all the triggers clear themselves. For
example, in case of commit all the on_commit triggers must clear
themselves, and the rollback triggers are destroyed. Vice versa
when a rollback happens. But it didn't work because the Lua
triggers all work via a common runner lbox_trigger_run(), which
can't destroy its argument in most of the cases (space:on_replace,
:before_replace, ...).
Closes #6025
---
Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-6025-box.on_commit-leak
Issue: https://github.com/tarantool/tarantool/issues/6025
.../unreleased/gh-6025-box.on_commit-leak.md | 3 +
src/box/alter.cc | 39 +++++++++++--
src/box/txn.c | 26 +++++++--
src/box/txn_limbo.c | 2 +
src/box/vinyl.c | 2 +
.../gh-6025-box.on_commit-leak.test.lua | 57 +++++++++++++++++++
6 files changed, 119 insertions(+), 10 deletions(-)
create mode 100644 changelogs/unreleased/gh-6025-box.on_commit-leak.md
create mode 100755 test/app-tap/gh-6025-box.on_commit-leak.test.lua
diff --git a/changelogs/unreleased/gh-6025-box.on_commit-leak.md b/changelogs/unreleased/gh-6025-box.on_commit-leak.md
new file mode 100644
index 000000000..36bcbe36e
--- /dev/null
+++ b/changelogs/unreleased/gh-6025-box.on_commit-leak.md
@@ -0,0 +1,3 @@
+## bugfix/core
+
+* Fix memory leak on each `box.on_commit()` and `box.on_rollback()` (gh-6025).
diff --git a/src/box/alter.cc b/src/box/alter.cc
index 6c0a131c0..449db9eb2 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -987,6 +987,7 @@ alter_space_commit(struct trigger *trigger, void *event)
{
struct txn *txn = (struct txn *) event;
struct alter_space *alter = (struct alter_space *) trigger->data;
+ trigger_clear(trigger);
/*
* The engine (vinyl) expects us to pass the signature of
* the row that performed this operation, not the signature
@@ -1031,6 +1032,7 @@ static int
alter_space_rollback(struct trigger *trigger, void * /* event */)
{
struct alter_space *alter = (struct alter_space *) trigger->data;
+ trigger_clear(trigger);
/* Rollback alter ops */
class AlterSpaceOp *op;
try {
@@ -1928,6 +1930,7 @@ on_drop_space_commit(struct trigger *trigger, void *event)
{
(void) event;
struct space *space = (struct space *)trigger->data;
+ trigger_clear(trigger);
space_delete(space);
return 0;
}
@@ -1942,6 +1945,7 @@ on_drop_space_rollback(struct trigger *trigger, void *event)
{
(void) event;
struct space *space = (struct space *)trigger->data;
+ trigger_clear(trigger);
space_cache_replace(NULL, space);
return 0;
}
@@ -2094,6 +2098,7 @@ on_create_view_commit(struct trigger *trigger, void *event)
{
(void) event;
struct Select *select = (struct Select *)trigger->data;
+ trigger_clear(trigger);
sql_select_delete(sql_get(), select);
return 0;
}
@@ -2108,6 +2113,7 @@ on_create_view_rollback(struct trigger *trigger, void *event)
{
(void) event;
struct Select *select = (struct Select *)trigger->data;
+ trigger_clear(trigger);
update_view_references(select, -1, true, NULL);
sql_select_delete(sql_get(), select);
return 0;
@@ -2123,6 +2129,7 @@ on_drop_view_commit(struct trigger *trigger, void *event)
{
(void) event;
struct Select *select = (struct Select *)trigger->data;
+ trigger_clear(trigger);
sql_select_delete(sql_get(), select);
return 0;
}
@@ -2137,6 +2144,7 @@ on_drop_view_rollback(struct trigger *trigger, void *event)
{
(void) event;
struct Select *select = (struct Select *)trigger->data;
+ trigger_clear(trigger);
update_view_references(select, 1, true, NULL);
sql_select_delete(sql_get(), select);
return 0;
@@ -3465,6 +3473,7 @@ on_drop_func_commit(struct trigger *trigger, void * /* event */)
{
/* Delete the old function. */
struct func *func = (struct func *)trigger->data;
+ trigger_clear(trigger);
func_delete(func);
return 0;
}
@@ -3474,6 +3483,7 @@ on_drop_func_rollback(struct trigger *trigger, void * /* event */)
{
/* Insert the old function back into the cache. */
struct func *func = (struct func *)trigger->data;
+ trigger_clear(trigger);
func_cache_insert(func);
if (trigger_run(&on_alter_func, func) != 0)
return -1;
@@ -3715,6 +3725,7 @@ on_drop_collation_commit(struct trigger *trigger, void *event)
{
(void) event;
struct coll_id *coll_id = (struct coll_id *) trigger->data;
+ trigger_clear(trigger);
coll_id_delete(coll_id);
return 0;
}
@@ -3725,6 +3736,7 @@ on_drop_collation_rollback(struct trigger *trigger, void *event)
{
(void) event;
struct coll_id *coll_id = (struct coll_id *) trigger->data;
+ trigger_clear(trigger);
struct coll_id *replaced_id;
if (coll_id_cache_replace(coll_id, &replaced_id) != 0)
panic("Out of memory on insertion into collation cache");
@@ -4187,6 +4199,7 @@ static int
register_replica(struct trigger *trigger, void * /* event */)
{
struct tuple *new_tuple = (struct tuple *)trigger->data;
+ trigger_clear(trigger);
uint32_t id;
if (tuple_field_u32(new_tuple, BOX_CLUSTER_FIELD_ID, &id) != 0)
return -1;
@@ -4211,7 +4224,7 @@ static int
unregister_replica(struct trigger *trigger, void * /* event */)
{
struct tuple *old_tuple = (struct tuple *)trigger->data;
-
+ trigger_clear(trigger);
struct tt_uuid old_uuid;
if (tuple_field_uuid(old_tuple, BOX_CLUSTER_FIELD_UUID, &old_uuid) != 0)
return -1;
@@ -4392,6 +4405,7 @@ on_drop_sequence_commit(struct trigger *trigger, void * /* event */)
{
/* Delete the old sequence. */
struct sequence *seq = (struct sequence *)trigger->data;
+ trigger_clear(trigger);
sequence_delete(seq);
return 0;
}
@@ -4401,6 +4415,7 @@ on_drop_sequence_rollback(struct trigger *trigger, void * /* event */)
{
/* Insert the old sequence back into the cache. */
struct sequence *seq = (struct sequence *)trigger->data;
+ trigger_clear(trigger);
sequence_cache_insert(seq);
if (trigger_run(&on_alter_sequence, seq) != 0)
return -1;
@@ -4413,6 +4428,7 @@ on_alter_sequence_commit(struct trigger *trigger, void * /* event */)
{
/* Delete the old old sequence definition. */
struct sequence_def *def = (struct sequence_def *)trigger->data;
+ trigger_clear(trigger);
free(def);
return 0;
}
@@ -4422,6 +4438,7 @@ on_alter_sequence_rollback(struct trigger *trigger, void * /* event */)
{
/* Restore the old sequence definition. */
struct sequence_def *def = (struct sequence_def *)trigger->data;
+ trigger_clear(trigger);
struct sequence *seq = sequence_by_id(def->id);
assert(seq != NULL);
free(seq->def);
@@ -4837,6 +4854,7 @@ on_drop_trigger_rollback(struct trigger *trigger, void * /* event */)
{
struct sql_trigger *old_trigger = (struct sql_trigger *)trigger->data;
struct sql_trigger *new_trigger;
+ trigger_clear(trigger);
if (old_trigger == NULL)
return 0;
if (sql_trigger_replace(sql_trigger_name(old_trigger),
@@ -4856,6 +4874,7 @@ on_replace_trigger_rollback(struct trigger *trigger, void * /* event */)
{
struct sql_trigger *old_trigger = (struct sql_trigger *)trigger->data;
struct sql_trigger *new_trigger;
+ trigger_clear(trigger);
if (sql_trigger_replace(sql_trigger_name(old_trigger),
sql_trigger_space_id(old_trigger),
old_trigger, &new_trigger) != 0)
@@ -4872,6 +4891,7 @@ static int
on_replace_trigger_commit(struct trigger *trigger, void * /* event */)
{
struct sql_trigger *old_trigger = (struct sql_trigger *)trigger->data;
+ trigger_clear(trigger);
sql_trigger_delete(sql_get(), old_trigger);
return 0;
}
@@ -5209,6 +5229,7 @@ on_create_fk_constraint_rollback(struct trigger *trigger, void *event)
{
(void) event;
struct fk_constraint *fk = (struct fk_constraint *)trigger->data;
+ trigger_clear(trigger);
rlist_del_entry(fk, in_parent_space);
rlist_del_entry(fk, in_child_space);
struct space *child = space_by_id(fk->def->child_id);
@@ -5226,6 +5247,7 @@ on_replace_fk_constraint_rollback(struct trigger *trigger, void *event)
{
(void) event;
struct fk_constraint *old_fk = (struct fk_constraint *)trigger->data;
+ trigger_clear(trigger);
struct space *parent = space_by_id(old_fk->def->parent_id);
struct space *child = space_by_id(old_fk->def->child_id);
struct fk_constraint *new_fk =
@@ -5245,6 +5267,7 @@ on_drop_fk_constraint_rollback(struct trigger *trigger, void *event)
{
(void) event;
struct fk_constraint *old_fk = (struct fk_constraint *)trigger->data;
+ trigger_clear(trigger);
struct space *parent = space_by_id(old_fk->def->parent_id);
struct space *child = space_by_id(old_fk->def->child_id);
if (space_insert_constraint_id(child, CONSTRAINT_TYPE_FK,
@@ -5271,6 +5294,7 @@ on_drop_or_replace_fk_constraint_commit(struct trigger *trigger, void *event)
{
(void) event;
fk_constraint_delete((struct fk_constraint *) trigger->data);
+ trigger_clear(trigger);
return 0;
}
@@ -5312,8 +5336,9 @@ error:
/** A trigger invoked on replace in the _fk_constraint space. */
static int
-on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event)
+on_replace_dd_fk_constraint(struct trigger *trigger, void *event)
{
+ trigger_clear(trigger);
struct txn *txn = (struct txn *) event;
struct txn_stmt *stmt = txn_current_stmt(txn);
struct tuple *old_tuple = stmt->old_tuple;
@@ -5570,6 +5595,7 @@ static int
on_create_ck_constraint_rollback(struct trigger *trigger, void * /* event */)
{
struct ck_constraint *ck = (struct ck_constraint *)trigger->data;
+ trigger_clear(trigger);
assert(ck != NULL);
struct space *space = space_by_id(ck->def->space_id);
assert(space != NULL);
@@ -5588,6 +5614,7 @@ static int
on_drop_ck_constraint_commit(struct trigger *trigger, void * /* event */)
{
struct ck_constraint *ck = (struct ck_constraint *)trigger->data;
+ trigger_clear(trigger);
assert(ck != NULL);
ck_constraint_delete(ck);
return 0;
@@ -5598,6 +5625,7 @@ static int
on_drop_ck_constraint_rollback(struct trigger *trigger, void * /* event */)
{
struct ck_constraint *ck = (struct ck_constraint *)trigger->data;
+ trigger_clear(trigger);
assert(ck != NULL);
struct space *space = space_by_id(ck->def->space_id);
assert(space != NULL);
@@ -5616,6 +5644,7 @@ static int
on_replace_ck_constraint_commit(struct trigger *trigger, void * /* event */)
{
struct ck_constraint *ck = (struct ck_constraint *)trigger->data;
+ trigger_clear(trigger);
if (ck != NULL)
ck_constraint_delete(ck);
return 0;
@@ -5626,6 +5655,7 @@ static int
on_replace_ck_constraint_rollback(struct trigger *trigger, void * /* event */)
{
struct ck_constraint *ck = (struct ck_constraint *)trigger->data;
+ trigger_clear(trigger);
assert(ck != NULL);
struct space *space = space_by_id(ck->def->space_id);
assert(space != NULL);
@@ -5642,8 +5672,9 @@ on_replace_ck_constraint_rollback(struct trigger *trigger, void * /* event */)
/** A trigger invoked on replace in the _ck_constraint space. */
static int
-on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event)
+on_replace_dd_ck_constraint(struct trigger *trigger, void *event)
{
+ trigger_clear(trigger);
struct txn *txn = (struct txn *) event;
struct txn_stmt *stmt = txn_current_stmt(txn);
struct tuple *old_tuple = stmt->old_tuple;
@@ -5775,7 +5806,7 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event)
static int
on_replace_dd_func_index(struct trigger *trigger, void *event)
{
- (void) trigger;
+ trigger_clear(trigger);
struct txn *txn = (struct txn *) event;
struct txn_stmt *stmt = txn_current_stmt(txn);
struct tuple *old_tuple = stmt->old_tuple;
diff --git a/src/box/txn.c b/src/box/txn.c
index 03b39e0de..4dfe1ad82 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -54,7 +54,7 @@ static int
txn_on_yield(struct trigger *trigger, void *event);
static void
-txn_run_rollback_triggers(struct txn *txn, struct rlist *triggers);
+txn_run_rollback_triggers(struct txn *txn);
static int
txn_add_redo(struct txn *txn, struct txn_stmt *stmt, struct request *request)
@@ -150,7 +150,7 @@ txn_rollback_one_stmt(struct txn *txn, struct txn_stmt *stmt)
if (txn->engine != NULL && stmt->space != NULL)
engine_rollback_statement(txn->engine, txn, stmt);
if (stmt->has_triggers)
- txn_run_rollback_triggers(txn, &stmt->on_rollback);
+ txn_run_rollback_triggers(txn);
}
static void
@@ -438,8 +438,9 @@ fail:
* A helper function to process on_commit triggers.
*/
static void
-txn_run_commit_triggers(struct txn *txn, struct rlist *triggers)
+txn_run_commit_triggers(struct txn *txn)
{
+ struct rlist *triggers = &txn->on_commit;
/*
* Commit triggers must be run in the same order they
* were added so that a trigger sees the changes done
@@ -454,14 +455,21 @@ txn_run_commit_triggers(struct txn *txn, struct rlist *triggers)
unreachable();
panic("commit trigger failed");
}
+ /*
+ * Destroy here, not in txn_free() so as not to try to walk the lists
+ * when the transaction has no any triggers.
+ */
+ trigger_destroy(triggers);
+ trigger_destroy(&txn->on_rollback);
}
/*
* A helper function to process on_rollback triggers.
*/
static void
-txn_run_rollback_triggers(struct txn *txn, struct rlist *triggers)
+txn_run_rollback_triggers(struct txn *txn)
{
+ struct rlist *triggers = &txn->on_rollback;
if (trigger_run(triggers, txn) != 0) {
/*
* As transaction couldn't handle a trigger error so
@@ -471,6 +479,12 @@ txn_run_rollback_triggers(struct txn *txn, struct rlist *triggers)
unreachable();
panic("rollback trigger failed");
}
+ /*
+ * Destroy here, not in txn_free() so as not to try to walk the lists
+ * when the transaction has no any triggers.
+ */
+ trigger_destroy(&txn->on_commit);
+ trigger_destroy(triggers);
}
/* A helper function to process on_wal_write triggers. */
@@ -523,7 +537,7 @@ txn_complete_fail(struct txn *txn)
if (txn->engine != NULL)
engine_rollback(txn->engine, txn);
if (txn_has_flag(txn, TXN_HAS_TRIGGERS))
- txn_run_rollback_triggers(txn, &txn->on_rollback);
+ txn_run_rollback_triggers(txn);
txn_free_or_wakeup(txn);
}
@@ -537,7 +551,7 @@ txn_complete_success(struct txn *txn)
if (txn->engine != NULL)
engine_commit(txn->engine, txn);
if (txn_has_flag(txn, TXN_HAS_TRIGGERS))
- txn_run_commit_triggers(txn, &txn->on_commit);
+ txn_run_commit_triggers(txn);
txn_free_or_wakeup(txn);
}
diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
index c22bd6665..7d5ece81e 100644
--- a/src/box/txn_limbo.c
+++ b/src/box/txn_limbo.c
@@ -578,6 +578,7 @@ txn_commit_cb(struct trigger *trigger, void *event)
(void)event;
struct confirm_waitpoint *cwp =
(struct confirm_waitpoint *)trigger->data;
+ trigger_clear(trigger);
cwp->is_confirm = true;
fiber_wakeup(cwp->caller);
return 0;
@@ -589,6 +590,7 @@ txn_rollback_cb(struct trigger *trigger, void *event)
(void)event;
struct confirm_waitpoint *cwp =
(struct confirm_waitpoint *)trigger->data;
+ trigger_clear(trigger);
cwp->is_rollback = true;
fiber_wakeup(cwp->caller);
return 0;
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index b53e97593..d381179d0 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -4326,6 +4326,7 @@ vy_deferred_delete_on_commit(struct trigger *trigger, void *event)
{
struct txn *txn = event;
struct vy_mem *mem = trigger->data;
+ trigger_clear(trigger);
/*
* Update dump_lsn so that we can skip dumped deferred
* DELETE statements on WAL recovery.
@@ -4342,6 +4343,7 @@ vy_deferred_delete_on_rollback(struct trigger *trigger, void *event)
{
(void)event;
struct vy_mem *mem = trigger->data;
+ trigger_clear(trigger);
/* Unpin the mem pinned in vy_deferred_delete_on_replace(). */
vy_mem_unpin(mem);
return 0;
diff --git a/test/app-tap/gh-6025-box.on_commit-leak.test.lua b/test/app-tap/gh-6025-box.on_commit-leak.test.lua
new file mode 100755
index 000000000..ec4e8b099
--- /dev/null
+++ b/test/app-tap/gh-6025-box.on_commit-leak.test.lua
@@ -0,0 +1,57 @@
+#!/usr/bin/env tarantool
+
+--
+-- gh-6025: box.on_commit() and box.on_rollback() triggers always leaked.
+--
+
+local tap = require('tap')
+
+--
+-- Create a functional reference at the passed value. Not at the variable
+-- keeping it in the caller, but at the value itself.
+--
+local function wrap_value(value)
+ return function()
+ assert(value == nil or value ~= nil)
+ end
+end
+
+local function test_on_commit_rollback(test)
+ test:plan(2)
+
+ local s = box.schema.create_space('test')
+ s:create_index('pk')
+ local weak_ref = setmetatable({}, {__mode = 'v'})
+
+ -- If the triggers are deleted, the wrapped value reference must disappear
+ -- and nothing should keep the value from collection from the weak table.
+ local value = {}
+ weak_ref[1] = value
+ box.begin()
+ s:replace{1}
+ box.on_commit(wrap_value(value))
+ box.on_rollback(wrap_value(value))
+ box.commit()
+ value = nil
+ collectgarbage()
+ test:isnil(weak_ref[1], 'on commit both triggers are deleted')
+
+ value = {}
+ weak_ref[1] = value
+ box.begin()
+ s:replace{2}
+ box.on_commit(wrap_value(value))
+ box.on_rollback(wrap_value(value))
+ box.rollback()
+ value = nil
+ collectgarbage()
+ test:isnil(weak_ref[1], 'on rollback both triggers are deleted')
+end
+
+box.cfg{}
+
+local test = tap.test('gh-6025-box.on_commit-leak')
+test:plan(1)
+test:test('delete triggers on commit and rollback', test_on_commit_rollback)
+
+os.exit(test:check() and 0 or 1)
--
2.24.3 (Apple Git-128)
More information about the Tarantool-patches
mailing list