[Tarantool-patches] [PATCH 0/2] fix replication/anon test segfaut

[Tarantool-patches] [PATCH 0/2] fix replication/anon test segfaut

[Serge Petrenko]

https://github.com/tarantool/tarantool/issues/4731
https://github.com/tarantool/tarantool/tree/sp/gh-4731-anon-segfault-full-ci

@ChangeLog
  - fix segmentation fault on master side when one of the
    replicas transitions from anonymous to normal (gh-4731)

Serge Petrenko (2):
  trigger: add trigger_is_set() method
  relay: fix  segfault on replica transition from anonymous

 src/box/relay.cc       | 16 +++++++++++-----
 src/lib/core/trigger.h |  6 ++++++
 2 files changed, 17 insertions(+), 5 deletions(-)

-- 
2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH 1/2] trigger: add trigger_is_set() method

[Serge Petrenko]

---
 src/lib/core/trigger.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/lib/core/trigger.h b/src/lib/core/trigger.h
index 7b500dd92..ae477ad70 100644
--- a/src/lib/core/trigger.h
+++ b/src/lib/core/trigger.h
@@ -95,6 +95,12 @@ trigger_add_unique(struct rlist *list, struct trigger *trigger)
 	trigger_add(list, trigger);
 }
 
+static inline int
+trigger_is_set(struct trigger *trigger)
+{
+	return !rlist_empty(&trigger->link);
+}
+
 static inline void
 trigger_clear(struct trigger *trigger)
 {
-- 
2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH 2/2] relay: fix segfault on replica transition from anonymous

[Serge Petrenko]

relay_subscribe_f sets a recovery trigger notifying tx when a full log
is read and gc consumer corresponding to the replica may be advanced.
Since anonymous replicas do not have gc consumers, the trigger isn't
added for them. However, on relay exit, the trigger deletion depends
on replica->anon flag. This is buggy in case relay stalls on exit due to
replica disconnect. Replica has time to reconnect and register as a
normal instance, hence its replica->anon flag will be false by the time
we check whether to clear triggers or not, effectively makinig us to
clear unset triggers and segfault.

Fix this by actually checking whether the trigger is set instead of
relying on replica->anon flag.

Closes #4731
---
 src/box/relay.cc | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/src/box/relay.cc b/src/box/relay.cc
index c634348a4..5f6edcd78 100644
--- a/src/box/relay.cc
+++ b/src/box/relay.cc
@@ -580,9 +580,8 @@ relay_subscribe_f(va_list ap)
 	 * Not needed for anonymous replicas, since they
 	 * aren't registered with gc at all.
 	 */
-	struct trigger on_close_log = {
-		RLIST_LINK_INITIALIZER, relay_on_close_log_f, relay, NULL
-	};
+	struct trigger on_close_log;
+	trigger_create(&on_close_log, relay_on_close_log_f, relay, NULL);
 	if (!relay->replica->anon)
 		trigger_add(&r->on_close_log, &on_close_log);
 
@@ -662,8 +661,15 @@ relay_subscribe_f(va_list ap)
 	diag_log();
 	say_crit("exiting the relay loop");
 
-	/* Clear garbage collector trigger and WAL watcher. */
-	if (!relay->replica->anon)
+	/*
+	 * Clear garbage collector trigger and WAL watcher.
+	 * Note, even though we set the  trigger only when the
+	 * replica is anonymous, we cannot check replica->anon
+	 * here to  determine whether the trigger is set. The
+	 * replica may have registered and become non-anonymous
+	 * while this relay thread was still exiting.
+	 */
+	if (trigger_is_set(&on_close_log))
 		trigger_clear(&on_close_log);
 	wal_clear_watcher(&relay->wal_watcher, cbus_process);
 
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH 1/2] trigger: add trigger_is_set() method

[Cyrill Gorcunov]

On Sat, Apr 11, 2020 at 03:33:44PM +0300, Serge Petrenko wrote:
> ---
>  src/lib/core/trigger.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/lib/core/trigger.h b/src/lib/core/trigger.h
> index 7b500dd92..ae477ad70 100644
> --- a/src/lib/core/trigger.h
> +++ b/src/lib/core/trigger.h
> @@ -95,6 +95,12 @@ trigger_add_unique(struct rlist *list, struct trigger *trigger)
>  	trigger_add(list, trigger);
>  }
>  
> +static inline int
> +trigger_is_set(struct trigger *trigger)
> +{
> +	return !rlist_empty(&trigger->link);
> +}
> +

Wait. Serge, you know I don;t like it. Can't we rather provide
normal static initializer say like

---
diff --git a/src/lib/core/trigger.h b/src/lib/core/trigger.h
index 7b500dd92..e9c6dd17e 100644
--- a/src/lib/core/trigger.h
+++ b/src/lib/core/trigger.h
@@ -59,6 +59,14 @@ struct trigger
        trigger_f0 destroy;
 };
 
+#define DECLARE_TRIGGER(__name, __run, __data, __destroy)      \
+       struct trigger __name = {                               \
+               .link   = RLIST_HEAD_INITIALIZER(__name.link),  \
+               .run    = __run,                                \
+               .data   = __data,                               \
+               .destroy= __destroy,                            \
+       }
+

and simply declare the trigger

diff --git a/src/box/relay.cc b/src/box/relay.cc
index c634348a4..cdc20dc5b 100644
--- a/src/box/relay.cc
+++ b/src/box/relay.cc
@@ -580,9 +580,7 @@ relay_subscribe_f(va_list ap)
         * Not needed for anonymous replicas, since they
         * aren't registered with gc at all.
         */
-       struct trigger on_close_log = {
-               RLIST_LINK_INITIALIZER, relay_on_close_log_f, relay, NULL
-       };
+       DECLARE_TRIGGER(on_close_log, relay_on_close_log_f, relay, NULL);
        if (!relay->replica->anon)
                trigger_add(&r->on_close_log, &on_close_log);

so that trigger_clear won't fail without if statement?

Re: [Tarantool-patches] [PATCH 1/2] trigger: add trigger_is_set() method

[Serge Petrenko]

> 11 апр. 2020 г., в 19:33, Cyrill Gorcunov <gorcunov@gmail.com> написал(а):
> 
> On Sat, Apr 11, 2020 at 03:33:44PM +0300, Serge Petrenko wrote:
>> ---
>> src/lib/core/trigger.h | 6 ++++++
>> 1 file changed, 6 insertions(+)
>> 
>> diff --git a/src/lib/core/trigger.h b/src/lib/core/trigger.h
>> index 7b500dd92..ae477ad70 100644
>> --- a/src/lib/core/trigger.h
>> +++ b/src/lib/core/trigger.h
>> @@ -95,6 +95,12 @@ trigger_add_unique(struct rlist *list, struct trigger *trigger)
>> 	trigger_add(list, trigger);
>> }
>> 
>> +static inline int
>> +trigger_is_set(struct trigger *trigger)
>> +{
>> +	return !rlist_empty(&trigger->link);
>> +}
>> +
> 
> Wait. Serge, you know I don;t like it. Can't we rather provide
> normal static initializer say like

Hi! Thanks for the review!

> 
> ---
> diff --git a/src/lib/core/trigger.h b/src/lib/core/trigger.h
> index 7b500dd92..e9c6dd17e 100644
> --- a/src/lib/core/trigger.h
> +++ b/src/lib/core/trigger.h
> @@ -59,6 +59,14 @@ struct trigger
>        trigger_f0 destroy;
> };
> 
> +#define DECLARE_TRIGGER(__name, __run, __data, __destroy)      \
> +       struct trigger __name = {                               \
> +               .link   = RLIST_HEAD_INITIALIZER(__name.link),  \
> +               .run    = __run,                                \
> +               .data   = __data,                               \
> +               .destroy= __destroy,                            \
> +       }
> +
> 
> and simply declare the trigger
> 
> diff --git a/src/box/relay.cc b/src/box/relay.cc
> index c634348a4..cdc20dc5b 100644
> --- a/src/box/relay.cc
> +++ b/src/box/relay.cc
> @@ -580,9 +580,7 @@ relay_subscribe_f(va_list ap)
>         * Not needed for anonymous replicas, since they
>         * aren't registered with gc at all.
>         */
> -       struct trigger on_close_log = {
> -               RLIST_LINK_INITIALIZER, relay_on_close_log_f, relay, NULL
> -       };
> +       DECLARE_TRIGGER(on_close_log, relay_on_close_log_f, relay, NULL);
>        if (!relay->replica->anon)
>                trigger_add(&r->on_close_log, &on_close_log);
> 
> so that trigger_clear won't fail without if statement?

This is doing the same as trigger_create(&&on_close_log);
Let’s just omit the if statement then together with the patch
adding trigger_is_set()?



--
Serge Petrenko
sergepetrenko@tarantool.org

Re: [Tarantool-patches] [PATCH 1/2] trigger: add trigger_is_set() method

[Cyrill Gorcunov]

On Mon, Apr 13, 2020 at 12:53:45PM +0300, Serge Petrenko wrote:
> > 
> > so that trigger_clear won't fail without if statement?
> 
> This is doing the same as trigger_create(&&on_close_log);

Yes, except it eliminates redundant trigger_create call :)

> Let’s just omit the if statement then together with the patch
> adding trigger_is_set()?

Yeah, sounds good, lets do it.