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

[Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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