From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 4AF8C6FC8B; Sun, 25 Apr 2021 18:42:39 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 4AF8C6FC8B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1619365359; bh=YpndQLE1K/LetnhtdXUgXRcCm0Yhj/xYsxjxK2skqH0=; h=To:Date:Subject:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=eOul35ILyA9/g8Myc554dBOxGnng6Amshz3WAQorbjfD4ubQJ6M1/Emb282huA2dt HE/Ryb65BZEqhLhrYywZ6I70cd+8Pm55pXJt83/43lV9UaG/DdUA5v7P3cTc6uLjiV LBcu6nF8dcnIt14yVZvu3lRPkNnKZEYtRC8qgVNI= Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 5B1616FC86 for ; Sun, 25 Apr 2021 18:42:37 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 5B1616FC86 Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1lagu4-0002OG-Af; Sun, 25 Apr 2021 18:42:36 +0300 To: tarantool-patches@dev.tarantool.org, gorcunov@gmail.com, sergepetrenko@tarantool.org Date: Sun, 25 Apr 2021 17:42:35 +0200 Message-Id: X-Mailer: git-send-email 2.24.3 (Apple Git-128) MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9203E2ABA940B75487ADF04F292A7BD7756ABB318AC2F9BF9182A05F538085040C3B1AF70D1EB74F09C8FB6F0D5DA0CA36343409004DABCE05099C8AE9120DF8C X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE75C94F75ECF5F42A4EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063770398A047C76876C8638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B2E51B6869BBAAAE73DD82BB00185A874311D9A310A4A5EBA7D2E47CDBA5A96583C09775C1D3CA48CFED8438A78DFE0A9E117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE78A0F7C24A37A3D769FA2833FD35BB23DF004C906525384302BEBFE083D3B9BA71A620F70A64A45A98AA50765F79006372E808ACE2090B5E1725E5C173C3A84C3C5EA940A35A165FF2DBA43225CD8A89FB26E97DCB74E625235872C767BF85DA2F004C90652538430E4A6367B16DE6309 X-B7AD71C0: 2623F767319EFA42AC98609FCEE262F9192335DD689A58EBAE0174B7F1092AFB5E9D0A81291C92A0E324922B6B9C87D2 X-C1DE0DAB: C20DE7B7AB408E4181F030C43753B8183A4AFAF3EA6BDC44C234C8B12C006B7AF13BB6B52C26B765200E1C6CAFA7A17287F19526EE2BB241B1881A6453793CE9C32612AADDFBE0612BCBB3E112654EA4C581CE5390BD6510A5FCC33AE53B6E92BB52A3B5A10F442104EBA3D8E7E5B87ABF8C51168CD8EBDB55D5BE2F85BDEC5FDC48ACC2A39D04F89CDFB48F4795C241BDAD6C7F3747799A X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3453741B480A6503C37C584F89104C522ED17D1926420840331246089100CFEE5EF203AD2DA5D843E01D7E09C32AA3244C8255D58818CC69D875F4B8D3615D4CCC1DD47778AE04E04DFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj08M52wfuxcHcJd2MJglCZw== X-Mailru-Sender: 689FA8AB762F73936BC43F508A0638227C156014B5476F7865F01D59ECFF2A923841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E267EA787935ED9F1B X-Mras: Ok Subject: [Tarantool-patches] [PATCH v2 1/1] txn: destroy commit and rollback triggers X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Vladislav Shpilevoy via Tarantool-patches Reply-To: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "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)