All triggers, visible to a user, crash on non-trivial changes

[Gerold103]

box.cfg{}
s = box.schema.create_space('test')
pk = s:create_index('pk')
t1 = function() print('t1') end
t2 = function() s:on_replace(nil, t1) s:on_replace(nil, t2) print('t2') end

s:on_replace(t1)
s:on_replace(t2)
s:replace{1}

This leads to surprising results. It could be a gamble to bet how that code will crash next time. Or will not crash. The only certain thing is that this code corrupts memory. While the example is not artificial. IMO a user should be able in a trigger to decide to drop all the triggers. Results are:

tarantool> s:replace{1}
t2
---
- error: attempt to call a nil value
...

Or

tarantool> s:replace{1}
t2
---
- [1]
...

Or

tarantool> s:replace{1}
t2
Process 83468 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_INSTRUCTION (code=EXC_I386_INVOP, subcode=0x0)
    frame #0: 0x0000000100150004 tarantool`tarantool_lua_init(tarantool_bin="?u\x80\x1d\x01", argc=1, argv=0x0000000104019038) at init.c:486:3
   483 			} else {
   484 				lua_pop(L, 1); /* nil */
   485 			}
-> 486 			lua_pop(L, 1); /* chunkname */
   487 		}
   488 		lua_pop(L, 1); /* _LOADED */
   489 	

Or anything else. This is because all public triggers are run with function trigger_run, which uses rlist_foreach_safe, but the latter can't withstand deletion of an element after current.

Possible solution - introduce struct rlist_safe, or even do not use rlist for triggers at all. When a one wants to delete a trigger, it should be marked for deletion, but deleted only on a next attempt to run, for example.

[Gerold103]

Affected places: space:on_replace, before_replace, box.on_commit, box.on_rollback, box.session.on_connect, and all the others using lbox_trigger_reset. They are public.

[alyapunov]

* Vladislav Shpilevoy <notifications@github.com> [19/06/01 06:03]:

Affected places: space:on_replace, before_replace, box.on_commit, box.on_rollback, box.session.on_connect, and all the others using lbox_trigger_reset. They are public.

I believe this is documented even. I remember discussing the problem with PeterG when triggers were introduced. IMO fixing this is just not worth it. We can simply state the behaviour is undefined. In Tarantool you can trivially crash the system (ffi.cast and dereference a null pointer), so we are not obliged to fix all of the crash scenarios just because it-is-a-crash.

…

-- Konstantin Osipov, Moscow, Russia

[Gerold103]

It is a bug, reproduced without any C manipulations. If you consider it not important, then you can move it to wishlist.

[Gerold103]

As I understand, Kirill considers it a bug, as I do. Here are my thoughts on how we could resolve it.
I thought about a new structure struct rlist_mutable, which is allowed to be totally mutable, be changed anyhow, which could allow to add or remove any item from any place any number of times.

It should contain a flag bool is_changed, which is set when the list is changed. During iteration over the list the cycle will check if the flag was set between iterations. If it did, then the cycle restarts, skips all the items until the current one, and continues normal operation.

Problems with my approach are

1. What to do, if exactly the current item was removed? We won't find it on re-roll. Because of the same reason we can't rely on any other items. Probably, we should skip till the current item and not more number of items than already scanned. Or add another flag bool is_seen to each item, which is set during iteration, and unset after. The latter is a problem, because it does not allow to iterate in multiple cycles, and it requires two iterations.
2. struct rlist_mutable probably won't be compatible with struct rlist, and we won't be able to use functions from rlist.h. But probably I am wrong here.

This mindblowing way is really hard to implement and support. There is another one, but less flexible. Lets define

struct rlist_mutable {
        struct base;
        int version;
};

Version is contained in the head only, incremented on each change. Before iteration the version is saved. If during iteration the version was changed, then abort the cycle. We will document that behaviour, that attempts to change a trigger list during iteration will stop its current processing.

[Totktonada]

I believe this is documented even.

I don't see anything about changing triggers inside triggers here and here.

@lenkis Can you verify and proceed with that?

[kyukhin]

IMHO, we need to ban this. No segfaults are allowed.

[kostja]

Kirill, this is contrary to triage guidelines. Please do not waste time on matters with low impact.

[locker]

Looks like documented undefined behavior:

I don't think we need to fix this, because:

1. Nobody requested this kind of functionality.
2. Implementing/fixing it would complicate the code.

[kyukhin]

This is documented UB, closing.

[sergepetrenko]

It's not necessary for a trigger to clear another trigger to crash. For example, a trigger may yield (this is allowed, I believe?) and while it yields some other code might clear the next-to-be-run trigger. This will result in a crash (actually, in an infinite loop).