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 363526FC86; Sun, 25 Apr 2021 11:44:50 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 363526FC86 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1619340290; bh=ZyFZQCYviIOitbof1NBLjnI6DPIaeTfQ0cyMvCZc1v8=; h=To:References:Date:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=PmUB+Vt5Xn6R0fYnOcmLmL05VwhsB8OG77tAAeG2LvYTXGgqkq/dZLLheXWKycu/n +bKzGIw9KAH+DTbt8dwYj0eWKvAULPEGw3Vg9Yv9t9LVyTWfpAuFgsMhupwWskABIb /dfdzzdV0hZSHorGG9MNss0IOYx0z/dfhrAA6J4g= Received: from smtp46.i.mail.ru (smtp46.i.mail.ru [94.100.177.106]) (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 3F8976FC86 for ; Sun, 25 Apr 2021 11:44:49 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 3F8976FC86 Received: by smtp46.i.mail.ru with esmtpa (envelope-from ) id 1laaNk-0006Uq-JA; Sun, 25 Apr 2021 11:44:49 +0300 To: Vladislav Shpilevoy , tarantool-patches@dev.tarantool.org, gorcunov@gmail.com References: <15101a51fcd1441813a384fef9f670150b522a3d.1619224125.git.v.shpilevoy@tarantool.org> Message-ID: Date: Sun, 25 Apr 2021 11:44:48 +0300 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.9.1 MIME-Version: 1.0 In-Reply-To: <15101a51fcd1441813a384fef9f670150b522a3d.1619224125.git.v.shpilevoy@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9203E2ABA940B7548DB5D504FB17EC32432A3176CEA2677EC182A05F5380850409DDEEFF674A917F3A50C7FAF876A003CA1FE53D9B4CD5B8F5A658D9737D70829 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7BDF1FA55CCD598A3EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637829D9538242026C38638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B2D0D8B221DEA8E4240E06B637B65D0D02267281413F282BCCD2E47CDBA5A96583C09775C1D3CA48CFE478A468B35FE767117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE7ABB305BD10C6E5099FA2833FD35BB23DF004C90652538430302FCEF25BFAB3454AD6D5ED66289B5278DA827A17800CE72B93559C52048E8AD32BA5DBAC0009BE395957E7521B51C20BC6067A898B09E4090A508E0FED6299176DF2183F8FC7C0B1CA5D0BF4193578B3661434B16C20AC78D18283394535A9E827F84554CEF5019E625A9149C048EE9ECD01F8117BC8BEE2021AF6380DFAD18AA50765F790063735872C767BF85DA227C277FBC8AE2E8BBFD80D3C1F6BEDE875ECD9A6C639B01B4E70A05D1297E1BBCB5012B2E24CD356 X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A23F051236DBC9DA6306C627C4D753511DAE0174B7F1092AFB5E9D0A81291C92A0A40BDE7E5DA29B11 X-C1DE0DAB: 0D63561A33F958A55CDD2F7809718710D4A78E1BEA4D746445C47CE76A09AE2BD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34D7D5115130EFC82428F08F09D092F7E301431B501C79B27537083D14A4F888F2BD1667E32F2539ED1D7E09C32AA3244C06797998129756E65826CC55F9FEACBAA90944CA99CF22E3FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj08M52wfuxcHimBB0RSCzvQ== X-Mailru-Sender: 583F1D7ACE8F49BDD2846D59FC20E9F8FCEC231191DC3E1289F9E104139722F3A03C9746225DBECC424AE0EB1F3D1D21E2978F233C3FAE6EE63DB1732555E4A8EE80603BA4A5B0BC112434F685709FCF0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH 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: Serge Petrenko via Tarantool-patches Reply-To: Serge Petrenko Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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