server: add wl_signal_emit_safe

Submitted by Simon Ser on July 13, 2018, 9:40 p.m.

Details

Message ID nzR-HnqoIgLL_CHQA78rx8xuKaqx8CBraCgnCYVQUdfiZMdhS0Om6SELMKJZbXUnKgFmcKs_hqL0lYmZfTET99EA07QVTJuuH90Ejfvf3JU=@emersion.fr
State New
Series "server: add wl_signal_emit_safe"
Headers show

Commit Message

Simon Ser July 13, 2018, 9:40 p.m.
This new function allows listeners to remove themselves or any
other listener when called. This version only works if listeners
are properly cleaned up when the wl_signal is free'd.

Signed-off-by: Simon Ser <contact@emersion.fr>
---
This is a [1] follow-up. Since we noticed the previous version is
not a drop-in replacement for wl_signal_emit, this patch just adds
a new function instead.

[1]: https://patchwork.freedesktop.org/patch/204641/

 src/wayland-server-core.h |  3 ++
 src/wayland-server.c      | 47 +++++++++++++++++++++
 tests/signal-test.c       | 86 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 136 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/wayland-server-core.h b/src/wayland-server-core.h
index 2e725d9..4a2948a 100644
--- a/src/wayland-server-core.h
+++ b/src/wayland-server-core.h
@@ -468,6 +468,9 @@  wl_signal_emit(struct wl_signal *signal, void *data)
 		l->notify(l, data);
 }
 
+void
+wl_signal_emit_safe(struct wl_signal *signal, void *data);
+
 typedef void (*wl_resource_destroy_func_t)(struct wl_resource *resource);
 
 /*
diff --git a/src/wayland-server.c b/src/wayland-server.c
index eae8d2e..a5f3735 100644
--- a/src/wayland-server.c
+++ b/src/wayland-server.c
@@ -1932,6 +1932,53 @@  wl_client_for_each_resource(struct wl_client *client,
 	wl_map_for_each(&client->objects, resource_iterator_helper, &context);
 }
 
+static void
+handle_noop(struct wl_listener *listener, void *data) {
+	/* Do nothing */
+}
+
+/** Emits this signal, safe against removal of any listener.
+ *
+ * \note This function can only be used if listeners are properly removed when
+ *       the wl_signal is free'd.
+ *
+ * \param signal The signal object that will emit the signal
+ * \param data The data that will be emitted with the signal
+ *
+ * \sa wl_signal_emit()
+ *
+ * \memberof wl_signal
+ */
+WL_EXPORT void
+wl_signal_emit_safe(struct wl_signal *signal, void *data) {
+	struct wl_listener cursor;
+	struct wl_listener end;
+
+	/* Add two special markers: one cursor and one end marker. This way, we know
+	 * that we've already called listeners on the left of the cursor and that we
+	 * don't want to call listeners on the right of the end marker. The 'it'
+	 * function can remove any element it wants from the list without troubles.
+	 * wl_list_for_each_safe tries to be safe but it fails: it works fine
+	 * if the current item is removed, but not if the next one is. */
+	wl_list_insert(&signal->listener_list, &cursor.link);
+	cursor.notify = handle_noop;
+	wl_list_insert(signal->listener_list.prev, &end.link);
+	end.notify = handle_noop;
+
+	while (cursor.link.next != &end.link) {
+		struct wl_list *pos = cursor.link.next;
+		struct wl_listener *l = wl_container_of(pos, l, link);
+
+		wl_list_remove(&cursor.link);
+		wl_list_insert(pos, &cursor.link);
+
+		l->notify(l, data);
+	}
+
+	wl_list_remove(&cursor.link);
+	wl_list_remove(&end.link);
+}
+
 /** \cond INTERNAL */
 
 /** Initialize a wl_priv_signal object
diff --git a/tests/signal-test.c b/tests/signal-test.c
index 7bbaa9f..dc762a4 100644
--- a/tests/signal-test.c
+++ b/tests/signal-test.c
@@ -115,3 +115,89 @@  TEST(signal_emit_to_more_listeners)
 
 	assert(3 * counter == count);
 }
+
+struct signal
+{
+	struct wl_signal signal;
+	struct wl_listener l1, l2, l3;
+	int count;
+	struct wl_listener *current;
+};
+
+static void notify_remove(struct wl_listener *l, void *data)
+{
+	struct signal *sig = data;
+	wl_list_remove(&sig->current->link);
+	wl_list_init(&sig->current->link);
+	sig->count++;
+}
+
+#define INIT \
+	wl_signal_init(&signal.signal); \
+	wl_list_init(&signal.l1.link); \
+	wl_list_init(&signal.l2.link); \
+	wl_list_init(&signal.l3.link);
+#define CHECK_EMIT(expected) \
+	signal.count = 0; \
+	wl_signal_emit_safe(&signal.signal, &signal); \
+	assert(signal.count == expected);
+
+TEST(signal_remove_listener)
+{
+	test_set_timeout(4);
+
+	struct signal signal;
+
+	signal.l1.notify = notify_remove;
+	signal.l2.notify = notify_remove;
+	signal.l3.notify = notify_remove;
+
+	INIT
+	wl_signal_add(&signal.signal, &signal.l1);
+
+	signal.current = &signal.l1;
+	CHECK_EMIT(1)
+	CHECK_EMIT(0)
+
+	INIT
+	wl_signal_add(&signal.signal, &signal.l1);
+	wl_signal_add(&signal.signal, &signal.l2);
+
+	CHECK_EMIT(2)
+	CHECK_EMIT(1)
+
+	INIT
+	wl_signal_add(&signal.signal, &signal.l1);
+	wl_signal_add(&signal.signal, &signal.l2);
+
+	signal.current = &signal.l2;
+	CHECK_EMIT(1)
+	CHECK_EMIT(1)
+
+	INIT
+	wl_signal_add(&signal.signal, &signal.l1);
+	wl_signal_add(&signal.signal, &signal.l2);
+	wl_signal_add(&signal.signal, &signal.l3);
+
+	signal.current = &signal.l1;
+	CHECK_EMIT(3)
+	CHECK_EMIT(2)
+
+	INIT
+	wl_signal_add(&signal.signal, &signal.l1);
+	wl_signal_add(&signal.signal, &signal.l2);
+	wl_signal_add(&signal.signal, &signal.l3);
+
+	signal.current = &signal.l2;
+	CHECK_EMIT(2)
+	CHECK_EMIT(2)
+
+	INIT
+	wl_signal_add(&signal.signal, &signal.l1);
+	wl_signal_add(&signal.signal, &signal.l2);
+	wl_signal_add(&signal.signal, &signal.l3);
+
+	signal.current = &signal.l3;
+	CHECK_EMIT(2)
+	CHECK_EMIT(2)
+}

Comments

wl@ongy.net July 14, 2018, 6:57 a.m.
On 2018/July/13 05:40, Simon Ser wrote:
> This new function allows listeners to remove themselves or any
> other listener when called. This version only works if listeners
> are properly cleaned up when the wl_signal is free'd.
This is about free()ing the wl_listener in the actual handler, not the 
wl_signal, I think the wording here (and in the code-comment in the patch) is 
off.

Also I'd say that the commit message needs to be a bit longer. While "we" 
(anyone who participated in that earlier thread) know what this is about and 
why we need it, someone who looks at this patch and at wl_signal_emit which 
uses wl_list_for_each*_safe* will be confused bout what's going on.
Ah, I just saw it's in the comment of the function, I'd still move that to the 
doxygen comment, or commit message and out of the pure code comment
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> ---
> This is a [1] follow-up. Since we noticed the previous version is
> not a drop-in replacement for wl_signal_emit, this patch just adds
> a new function instead.
> 
> [1]: https://patchwork.freedesktop.org/patch/204641/
> 
>  src/wayland-server-core.h |  3 ++
>  src/wayland-server.c      | 47 +++++++++++++++++++++
>  tests/signal-test.c       | 86 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 136 insertions(+)
> 
> diff --git a/src/wayland-server-core.h b/src/wayland-server-core.h
> index 2e725d9..4a2948a 100644
> --- a/src/wayland-server-core.h
> +++ b/src/wayland-server-core.h
> @@ -468,6 +468,9 @@ wl_signal_emit(struct wl_signal *signal, void *data)
>  		l->notify(l, data);
>  }
>  
> +void
> +wl_signal_emit_safe(struct wl_signal *signal, void *data);
> +
>  typedef void (*wl_resource_destroy_func_t)(struct wl_resource *resource);
>  
>  /*
> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index eae8d2e..a5f3735 100644
> --- a/src/wayland-server.c
> +++ b/src/wayland-server.c
> @@ -1932,6 +1932,53 @@ wl_client_for_each_resource(struct wl_client *client,
>  	wl_map_for_each(&client->objects, resource_iterator_helper, &context);
>  }
>  
> +static void
> +handle_noop(struct wl_listener *listener, void *data) {
> +	/* Do nothing */
> +}
> +
> +/** Emits this signal, safe against removal of any listener.
> + *
> + * \note This function can only be used if listeners are properly removed when
> + *       the wl_signal is free'd.
> + *
> + * \param signal The signal object that will emit the signal
> + * \param data The data that will be emitted with the signal
> + *
> + * \sa wl_signal_emit()
> + *
> + * \memberof wl_signal
> + */
> +WL_EXPORT void
> +wl_signal_emit_safe(struct wl_signal *signal, void *data) {
> +	struct wl_listener cursor;
> +	struct wl_listener end;
Semi-serious: We could also add something like:
	if (signal.listener_list.prev == signal.listener_list.next) {
		wl_signal_emit(signal);
		return;
	}

And get rid of that wl_priv_signal, since that supported exactly this little 
corner-case (as I showed with the additional test a while ago) and not the 
general asumption that it's safe not to remove a destroy listener in general.
> +
> +	/* Add two special markers: one cursor and one end marker. This way, we know
> +	 * that we've already called listeners on the left of the cursor and that we
> +	 * don't want to call listeners on the right of the end marker. The 'it'
> +	 * function can remove any element it wants from the list without troubles.
> +	 * wl_list_for_each_safe tries to be safe but it fails: it works fine
> +	 * if the current item is removed, but not if the next one is. */
> +	wl_list_insert(&signal->listener_list, &cursor.link);
> +	cursor.notify = handle_noop;
> +	wl_list_insert(signal->listener_list.prev, &end.link);
> +	end.notify = handle_noop;
> +
> +	while (cursor.link.next != &end.link) {
> +		struct wl_list *pos = cursor.link.next;
> +		struct wl_listener *l = wl_container_of(pos, l, link);
> +
> +		wl_list_remove(&cursor.link);
> +		wl_list_insert(pos, &cursor.link);
> +
> +		l->notify(l, data);
> +	}
> +
> +	wl_list_remove(&cursor.link);
> +	wl_list_remove(&end.link);
> +}
> +
>  /** \cond INTERNAL */
>  
>  /** Initialize a wl_priv_signal object
> diff --git a/tests/signal-test.c b/tests/signal-test.c
> index 7bbaa9f..dc762a4 100644
> --- a/tests/signal-test.c
> +++ b/tests/signal-test.c
> @@ -115,3 +115,89 @@ TEST(signal_emit_to_more_listeners)
>  
>  	assert(3 * counter == count);
>  }
> +
> +struct signal
> +{
> +	struct wl_signal signal;
> +	struct wl_listener l1, l2, l3;
> +	int count;
> +	struct wl_listener *current;
> +};
> +
> +static void notify_remove(struct wl_listener *l, void *data)
> +{
> +	struct signal *sig = data;
> +	wl_list_remove(&sig->current->link);
> +	wl_list_init(&sig->current->link);
> +	sig->count++;
> +}
> +
> +#define INIT \
> +	wl_signal_init(&signal.signal); \
> +	wl_list_init(&signal.l1.link); \
> +	wl_list_init(&signal.l2.link); \
> +	wl_list_init(&signal.l3.link);
> +#define CHECK_EMIT(expected) \
> +	signal.count = 0; \
> +	wl_signal_emit_safe(&signal.signal, &signal); \
> +	assert(signal.count == expected);
> +
> +TEST(signal_remove_listener)
> +{
> +	test_set_timeout(4);
> +
> +	struct signal signal;
> +
> +	signal.l1.notify = notify_remove;
> +	signal.l2.notify = notify_remove;
> +	signal.l3.notify = notify_remove;
> +
> +	INIT
> +	wl_signal_add(&signal.signal, &signal.l1);
> +
> +	signal.current = &signal.l1;
> +	CHECK_EMIT(1)
> +	CHECK_EMIT(0)
> +
> +	INIT
> +	wl_signal_add(&signal.signal, &signal.l1);
> +	wl_signal_add(&signal.signal, &signal.l2);
> +
> +	CHECK_EMIT(2)
> +	CHECK_EMIT(1)
> +
> +	INIT
> +	wl_signal_add(&signal.signal, &signal.l1);
> +	wl_signal_add(&signal.signal, &signal.l2);
> +
> +	signal.current = &signal.l2;
> +	CHECK_EMIT(1)
> +	CHECK_EMIT(1)
> +
> +	INIT
> +	wl_signal_add(&signal.signal, &signal.l1);
> +	wl_signal_add(&signal.signal, &signal.l2);
> +	wl_signal_add(&signal.signal, &signal.l3);
> +
> +	signal.current = &signal.l1;
> +	CHECK_EMIT(3)
> +	CHECK_EMIT(2)
> +
> +	INIT
> +	wl_signal_add(&signal.signal, &signal.l1);
> +	wl_signal_add(&signal.signal, &signal.l2);
> +	wl_signal_add(&signal.signal, &signal.l3);
> +
> +	signal.current = &signal.l2;
> +	CHECK_EMIT(2)
> +	CHECK_EMIT(2)
> +
> +	INIT
> +	wl_signal_add(&signal.signal, &signal.l1);
> +	wl_signal_add(&signal.signal, &signal.l2);
> +	wl_signal_add(&signal.signal, &signal.l3);
> +
> +	signal.current = &signal.l3;
> +	CHECK_EMIT(2)
> +	CHECK_EMIT(2)
> +}
> -- 
> 2.18.0
> 
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel