From: Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: tarantool-patches@dev.tarantool.org, gorcunov@gmail.com, sergepetrenko@tarantool.org Subject: [Tarantool-patches] [PATCH v2 1/1] txn: destroy commit and rollback triggers Date: Sun, 25 Apr 2021 17:42:35 +0200 [thread overview] Message-ID: <d215d9fd4d03a01d60ea0ba740a023692178479c.1619365139.git.v.shpilevoy@tarantool.org> (raw) 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)
next reply other threads:[~2021-04-25 15:42 UTC|newest] Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-04-25 15:42 Vladislav Shpilevoy via Tarantool-patches [this message] 2021-04-26 9:37 ` Serge Petrenko via Tarantool-patches 2021-04-26 10:13 ` Cyrill Gorcunov via Tarantool-patches 2021-04-26 21:03 ` Vladislav Shpilevoy via Tarantool-patches 2021-04-26 21:33 ` Cyrill Gorcunov via Tarantool-patches 2021-04-28 21:58 ` Vladislav Shpilevoy via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=d215d9fd4d03a01d60ea0ba740a023692178479c.1619365139.git.v.shpilevoy@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=gorcunov@gmail.com \ --cc=sergepetrenko@tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v2 1/1] txn: destroy commit and rollback triggers' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox