Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH 1/1] txn: destroy commit and rollback triggers
@ 2021-04-24  0:29 Vladislav Shpilevoy via Tarantool-patches
  2021-04-25  8:44 ` Serge Petrenko via Tarantool-patches
  0 siblings, 1 reply; 3+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-04-24  0:29 UTC (permalink / raw)
  To: tarantool-patches, gorcunov, sergepetrenko

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)


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [Tarantool-patches] [PATCH 1/1] txn: destroy commit and rollback triggers
  2021-04-24  0:29 [Tarantool-patches] [PATCH 1/1] txn: destroy commit and rollback triggers Vladislav Shpilevoy via Tarantool-patches
@ 2021-04-25  8:44 ` Serge Petrenko via Tarantool-patches
  2021-04-25 15:47   ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 1 reply; 3+ messages in thread
From: Serge Petrenko via Tarantool-patches @ 2021-04-25  8:44 UTC (permalink / raw)
  To: Vladislav Shpilevoy, tarantool-patches, gorcunov



24.04.2021 03:29, Vladislav Shpilevoy пишет:
> 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, ...).

Hi! Thanks for  the patch!
Please find some comments below.

>
> 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/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);
>   }


1. This piece ran stmt->on_rollback, and you run txn->on_rollback now.
    These are not the same, for example check txn_rollback_to_svp below.


>   
>   static void
> @@ -438,8 +438,9 @@ fail:

...

>   
> 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

...

> 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
...


2. There's also applier_txn_rollback_cb() in applier.cc
    You should probably  clear it as  well.


> 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

3. The test's cool, I wouldn't have come up with it myself.
    Shouldn't it be in box-tap though?

> @@ -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)

-- 
Serge Petrenko


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [Tarantool-patches] [PATCH 1/1] txn: destroy commit and rollback triggers
  2021-04-25  8:44 ` Serge Petrenko via Tarantool-patches
@ 2021-04-25 15:47   ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 0 replies; 3+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-04-25 15:47 UTC (permalink / raw)
  To: Serge Petrenko, tarantool-patches, gorcunov

Hi! Thanks for the review!

>> 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
>> @@ -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);
>>   }
> 
> 
> 1. This piece ran stmt->on_rollback, and you run txn->on_rollback now.
>    These are not the same, for example check txn_rollback_to_svp below.

Indeed, I added a bug here, thanks for noticing. In v2 I removed
these run_rollback/run_commit functions. For the statement triggers
I added an assertion that they don't have a 'destroy' method. Because
if they would have, on commit we would need to walk the statement
list and for each statement destroy its rollback triggers, which would
add notable complexity.

>> 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
> ...
> 
> 
> 2. There's also applier_txn_rollback_cb() in applier.cc
>    You should probably  clear it as  well.

In v2 I deleted all trigger clears.

>> 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
> 
> 3. The test's cool, I wouldn't have come up with it myself.
>    Shouldn't it be in box-tap though?

Yes, this is about box, and I didn't think of box-tap. Moved there now.

I've sent v2 since the patch is too different now and the diff would
look not very easy to understand.

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-04-25 15:47 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-24  0:29 [Tarantool-patches] [PATCH 1/1] txn: destroy commit and rollback triggers Vladislav Shpilevoy via Tarantool-patches
2021-04-25  8:44 ` Serge Petrenko via Tarantool-patches
2021-04-25 15:47   ` Vladislav Shpilevoy via Tarantool-patches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox