[Tarantool-patches] [PATCH v2 1/1] txn: destroy commit and rollback triggers
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Sun Apr 25 18:42:35 MSK 2021
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)
More information about the Tarantool-patches
mailing list