[Tarantool-patches] [PATCH v2 1/1] txn: destroy commit and rollback triggers

[Tarantool-patches] [PATCH v2 1/1] txn: destroy commit and rollback triggers

[Vladislav Shpilevoy via Tarantool-patches]

They were not deleted ever. Worked fine for DDL and replication,
for which they were introduced in 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.

The statement triggers on_commit/on_rollback are left intact since
they are private and never need deletion, but the patch adds
assertions in case they ever would need to be destroyed.

Another option was to force all the commit and rollback 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. This would allow
not to manually destroy on_commit triggers in case of commit. 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, ...). It
could be patched but requires to add some work to the Lua triggers
affecting all of them, which in total might be not better.

Closes #6025
---
Changes in v2:
- Fixed a bug about txn rollback triggers called at statement
  rollback, added a test;
- Removed attempts to destroy and clear statement triggers;

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/txn.c                                 | 80 ++++++++-----------
 src/box/txn.h                                 |  4 +
 .../gh-6025-box.on_commit-leak.test.lua       | 57 +++++++++++++
 test/engine/savepoint.result                  | 37 +++++++++
 test/engine/savepoint.test.lua                | 24 ++++++
 6 files changed, 157 insertions(+), 48 deletions(-)
 create mode 100644 changelogs/unreleased/gh-6025-box.on_commit-leak.md
 create mode 100755 test/box-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/txn.c b/src/box/txn.c
index 03b39e0de..2ae55b4e1 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -53,9 +53,6 @@ txn_on_stop(struct trigger *trigger, void *event);
 static int
 txn_on_yield(struct trigger *trigger, void *event);
 
-static void
-txn_run_rollback_triggers(struct txn *txn, struct rlist *triggers);
-
 static int
 txn_add_redo(struct txn *txn, struct txn_stmt *stmt, struct request *request)
 {
@@ -149,8 +146,11 @@ 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);
+	if (stmt->has_triggers && trigger_run(&stmt->on_rollback, txn) != 0) {
+		diag_log();
+		unreachable();
+		panic("statement rollback trigger failed");
+	}
 }
 
 static void
@@ -434,45 +434,6 @@ fail:
 	return -1;
 }
 
-/*
- * A helper function to process on_commit triggers.
- */
-static void
-txn_run_commit_triggers(struct txn *txn, struct rlist *triggers)
-{
-	/*
-	 * Commit triggers must be run in the same order they
-	 * were added so that a trigger sees the changes done
-	 * by previous triggers (this is vital for DDL).
-	 */
-	if (trigger_run_reverse(triggers, txn) != 0) {
-		/*
-		 * As transaction couldn't handle a trigger error so
-		 * there is no option except panic.
-		 */
-		diag_log();
-		unreachable();
-		panic("commit trigger failed");
-	}
-}
-
-/*
- * A helper function to process on_rollback triggers.
- */
-static void
-txn_run_rollback_triggers(struct txn *txn, struct rlist *triggers)
-{
-	if (trigger_run(triggers, txn) != 0) {
-		/*
-		 * As transaction couldn't handle a trigger error so
-		 * there is no option except panic.
-		 */
-		diag_log();
-		unreachable();
-		panic("rollback trigger failed");
-	}
-}
-
 /* A helper function to process on_wal_write triggers. */
 static void
 txn_run_wal_write_triggers(struct txn *txn)
@@ -522,8 +483,17 @@ txn_complete_fail(struct txn *txn)
 		txn_rollback_one_stmt(txn, stmt);
 	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);
+	if (txn_has_flag(txn, TXN_HAS_TRIGGERS)) {
+		if (trigger_run(&txn->on_rollback, txn) != 0) {
+			diag_log();
+			unreachable();
+			panic("transaction rollback trigger failed");
+		}
+		/* Can't rollback more than once. */
+		trigger_destroy(&txn->on_rollback);
+		/* Commit won't happen after rollback. */
+		trigger_destroy(&txn->on_commit);
+	}
 	txn_free_or_wakeup(txn);
 }
 
@@ -536,8 +506,22 @@ txn_complete_success(struct txn *txn)
 	txn->status = TXN_COMMITTED;
 	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);
+	if (txn_has_flag(txn, TXN_HAS_TRIGGERS)) {
+		/*
+		 * Commit triggers must be run in the same order they were added
+		 * so that a trigger sees the changes done by previous triggers
+		 * (this is vital for DDL).
+		 */
+		if (trigger_run_reverse(&txn->on_commit, txn) != 0) {
+			diag_log();
+			unreachable();
+			panic("transaction commit trigger failed");
+		}
+		/* Can't commit more than once. */
+		trigger_destroy(&txn->on_commit);
+		/* Rollback won't happen after commit. */
+		trigger_destroy(&txn->on_rollback);
+	}
 	txn_free_or_wakeup(txn);
 }
 
diff --git a/src/box/txn.h b/src/box/txn.h
index fdac30d91..a06aaea23 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -545,6 +545,8 @@ static inline void
 txn_stmt_on_commit(struct txn_stmt *stmt, struct trigger *trigger)
 {
 	txn_stmt_init_triggers(stmt);
+	/* Statement triggers are private and never have anything to free. */
+	assert(trigger->destroy == NULL);
 	trigger_add(&stmt->on_commit, trigger);
 }
 
@@ -552,6 +554,8 @@ static inline void
 txn_stmt_on_rollback(struct txn_stmt *stmt, struct trigger *trigger)
 {
 	txn_stmt_init_triggers(stmt);
+	/* Statement triggers are private and never have anything to free. */
+	assert(trigger->destroy == NULL);
 	trigger_add(&stmt->on_rollback, trigger);
 }
 
diff --git a/test/box-tap/gh-6025-box.on_commit-leak.test.lua b/test/box-tap/gh-6025-box.on_commit-leak.test.lua
new file mode 100755
index 000000000..ec4e8b099
--- /dev/null
+++ b/test/box-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)
diff --git a/test/engine/savepoint.result b/test/engine/savepoint.result
index c23645ce6..554f1a7b8 100644
--- a/test/engine/savepoint.result
+++ b/test/engine/savepoint.result
@@ -550,6 +550,43 @@ s:select{}
 ---
 - - [3]
 ...
+--
+-- Rollback of the statement shouldn't invoke rollback of the transaction
+-- triggers.
+--
+did_rollback = false
+---
+...
+box.begin()                                                                     \
+svp = box.savepoint()                                                           \
+s:replace{4}                                                                    \
+box.on_rollback(function() did_rollback = true assert(false) end)               \
+box.rollback_to_savepoint(svp)                                                  \
+box.commit()
+---
+...
+assert(not did_rollback)
+---
+- true
+...
+-- Try DDL too. It installs the flag in the statement that it has triggers.
+-- This should not change the behaviour.
+box.begin()                                                                     \
+svp = box.savepoint()                                                           \
+box.schema.create_space('test2', {engine = engine})                             \
+box.on_rollback(function() did_rollback = true assert(false) end)               \
+box.rollback_to_savepoint(svp)                                                  \
+box.commit()
+---
+...
+assert(not did_rollback)
+---
+- true
+...
+assert(not box.schema.space.test2)
+---
+- true
+...
 s:drop()
 ---
 ...
diff --git a/test/engine/savepoint.test.lua b/test/engine/savepoint.test.lua
index 186ef85f2..47c0819dd 100644
--- a/test/engine/savepoint.test.lua
+++ b/test/engine/savepoint.test.lua
@@ -277,4 +277,28 @@ box.commit()
 test_run:cmd("setopt delimiter ''");
 s:select{}
 
+--
+-- Rollback of the statement shouldn't invoke rollback of the transaction
+-- triggers.
+--
+did_rollback = false
+box.begin()                                                                     \
+svp = box.savepoint()                                                           \
+s:replace{4}                                                                    \
+box.on_rollback(function() did_rollback = true assert(false) end)               \
+box.rollback_to_savepoint(svp)                                                  \
+box.commit()
+assert(not did_rollback)
+
+-- Try DDL too. It installs the flag in the statement that it has triggers.
+-- This should not change the behaviour.
+box.begin()                                                                     \
+svp = box.savepoint()                                                           \
+box.schema.create_space('test2', {engine = engine})                             \
+box.on_rollback(function() did_rollback = true assert(false) end)               \
+box.rollback_to_savepoint(svp)                                                  \
+box.commit()
+assert(not did_rollback)
+assert(not box.schema.space.test2)
+
 s:drop()
-- 
2.24.3 (Apple Git-128)

Re: [Tarantool-patches] [PATCH v2 1/1] txn: destroy commit and rollback triggers

[Serge Petrenko via Tarantool-patches]

25.04.2021 18:42, Vladislav Shpilevoy пишет:
> They were not deleted ever. Worked fine for DDL and replication,
> for which they were introduced in 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.
>
> The statement triggers on_commit/on_rollback are left intact since
> they are private and never need deletion, but the patch adds
> assertions in case they ever would need to be destroyed.
>
> Another option was to force all the commit and rollback 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. This would allow
> not to manually destroy on_commit triggers in case of commit. 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, ...). It
> could be patched but requires to add some work to the Lua triggers
> affecting all of them, which in total might be not better.
>
> Closes #6025
> ---
> Changes in v2:
> - Fixed a bug about txn rollback triggers called at statement
>    rollback, added a test;
> - Removed attempts to destroy and clear statement triggers;
>
> Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-6025-box.on_commit-leak
> Issue: https://github.com/tarantool/tarantool/issues/6025
>

Hi! Thanks for the fixes! LGTM.

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH v2 1/1] txn: destroy commit and rollback triggers

[Cyrill Gorcunov via Tarantool-patches]

On Sun, Apr 25, 2021 at 05:42:35PM +0200, Vladislav Shpilevoy wrote:
>  static int
>  txn_add_redo(struct txn *txn, struct txn_stmt *stmt, struct request *request)
>  {
> @@ -149,8 +146,11 @@ 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);
> +	if (stmt->has_triggers && trigger_run(&stmt->on_rollback, txn) != 0) {
> +		diag_log();
> +		unreachable();
> +		panic("statement rollback trigger failed");
> +	}
>  }

Good catch, Vlad! Can we please eliminate these unreachable() calls while
you're modifying it? It is simply not needed (because it is rather a hint
for compiler and in case if the code is executing it leads to unpredicted
results so panic() is only right thing to do).

Re: [Tarantool-patches] [PATCH v2 1/1] txn: destroy commit and rollback triggers

[Vladislav Shpilevoy via Tarantool-patches]

Thanks for the review!

On 26.04.2021 12:13, Cyrill Gorcunov wrote:
> On Sun, Apr 25, 2021 at 05:42:35PM +0200, Vladislav Shpilevoy wrote:
>>  static int
>>  txn_add_redo(struct txn *txn, struct txn_stmt *stmt, struct request *request)
>>  {
>> @@ -149,8 +146,11 @@ 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);
>> +	if (stmt->has_triggers && trigger_run(&stmt->on_rollback, txn) != 0) {
>> +		diag_log();
>> +		unreachable();
>> +		panic("statement rollback trigger failed");
>> +	}
>>  }
> 
> Good catch, Vlad! Can we please eliminate these unreachable() calls while
> you're modifying it? It is simply not needed (because it is rather a hint
> for compiler and in case if the code is executing it leads to unpredicted
> results so panic() is only right thing to do).

Np:

====================
diff --git a/src/box/txn.c b/src/box/txn.c
index 2ae55b4e1..869e43d02 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -148,7 +148,6 @@ txn_rollback_one_stmt(struct txn *txn, struct txn_stmt *stmt)
 		engine_rollback_statement(txn->engine, txn, stmt);
 	if (stmt->has_triggers && trigger_run(&stmt->on_rollback, txn) != 0) {
 		diag_log();
-		unreachable();
 		panic("statement rollback trigger failed");
 	}
 }
@@ -486,7 +485,6 @@ txn_complete_fail(struct txn *txn)
 	if (txn_has_flag(txn, TXN_HAS_TRIGGERS)) {
 		if (trigger_run(&txn->on_rollback, txn) != 0) {
 			diag_log();
-			unreachable();
 			panic("transaction rollback trigger failed");
 		}
 		/* Can't rollback more than once. */
@@ -514,7 +512,6 @@ txn_complete_success(struct txn *txn)
 		 */
 		if (trigger_run_reverse(&txn->on_commit, txn) != 0) {
 			diag_log();
-			unreachable();
 			panic("transaction commit trigger failed");
 		}
 		/* Can't commit more than once. */

Re: [Tarantool-patches] [PATCH v2 1/1] txn: destroy commit and rollback triggers

[Cyrill Gorcunov via Tarantool-patches]

On Mon, Apr 26, 2021 at 11:03:39PM +0200, Vladislav Shpilevoy wrote:
> 
> Np:
>

Thank you! Ack! 

Re: [Tarantool-patches] [PATCH v2 1/1] txn: destroy commit and rollback triggers

[Vladislav Shpilevoy via Tarantool-patches]

Pushed to master, 2.8, 2.7, and 1.10.