Merge pull request atomvm#760 from pguyot/w33/fix-concurrency-poll-s

Fix concurrency issue with poll and select events

List of select events could be modified while building or rebuilding the set of
poll fd.

These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).

SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later

Author: bettio

src/libAtomVM/resources.c

@@ -266,15 +266,14 @@ static inline void select_event_destroy(struct SelectEvent *select_event, Global
     free((void *) select_event);
 }
 
-void select_event_count_and_destroy_closed(size_t *read, size_t *write, size_t *either, GlobalContext *global)
+void select_event_count_and_destroy_closed(struct ListHead *select_events, size_t *read, size_t *write, size_t *either, GlobalContext *global)
 {
     size_t read_count = 0;
     size_t write_count = 0;
     size_t either_count = 0;
 
     struct ListHead *item;
     struct ListHead *tmp;
-    struct ListHead *select_events = synclist_wrlock(&global->select_events);
     MUTABLE_LIST_FOR_EACH (item, tmp, select_events) {
         struct SelectEvent *select_event = GET_LIST_ENTRY(item, struct SelectEvent, head);
         if (select_event->close) {
@@ -292,7 +291,6 @@ void select_event_count_and_destroy_closed(size_t *read, size_t *write, size_t *
             }
         }
     }
-    synclist_unlock(&global->select_events);
     if (read) {
         *read = read_count;
     }

src/libAtomVM/resources.h

@@ -102,12 +102,13 @@ bool select_event_notify(ErlNifEvent event, bool is_read, bool is_write, GlobalC
  * events marked for close.
  * @details Convenience function that can be called by `sys_poll_events` and
  * iterates on events to be closed and count them.
+ * @param select_events list of events, with a write lock
  * @param read on output number of events with read = 1, can be NULL
  * @param write on output number of events with write = 1, can be NULL
  * @param either on output number of events with either read = 1 or write = 1, can be NULL
  * @param global the global context
  */
-void select_event_count_and_destroy_closed(size_t *read, size_t *write, size_t *either, GlobalContext *global);
+void select_event_count_and_destroy_closed(struct ListHead *select_events, size_t *read, size_t *write, size_t *either, GlobalContext *global);
 
 /**
  * @brief Destroy monitors associated with a resource.

src/platforms/esp32/components/avm_sys/sys.c

@@ -627,9 +627,10 @@ static void *select_thread_loop(void *arg)
         int fd_index;
         if (select_events_poll_count < 0) {
             // Means it is dirty and should be rebuilt.
+            struct ListHead *select_events = synclist_wrlock(&glb->select_events);
             size_t select_events_new_count;
             if (select_events_poll_count < 0) {
-                select_event_count_and_destroy_closed(NULL, NULL, &select_events_new_count, glb);
+                select_event_count_and_destroy_closed(select_events, NULL, NULL, &select_events_new_count, glb);
             } else {
                 select_events_new_count = select_events_poll_count;
             }
@@ -643,7 +644,6 @@ static void *select_thread_loop(void *arg)
             fd_index = poll_count;
 
             struct ListHead *item;
-            struct ListHead *select_events = synclist_rdlock(&glb->select_events);
             LIST_FOR_EACH (item, select_events) {
                 struct SelectEvent *select_event = GET_LIST_ENTRY(item, struct SelectEvent, head);
                 if (select_event->read || select_event->write) {

src/platforms/generic_unix/lib/sys.c

@@ -117,7 +117,9 @@ static inline void sys_poll_events_with_kqueue(GlobalContext *glb, int timeout_m
     int select_events_poll_count = platform->select_events_poll_count;
     if (select_events_poll_count < 0) {
         size_t either;
-        select_event_count_and_destroy_closed(NULL, NULL, &either, glb);
+        struct ListHead *select_events = synclist_wrlock(&glb->select_events);
+        select_event_count_and_destroy_closed(select_events, NULL, NULL, &either, glb);
+        synclist_unlock(&glb->select_events);
         select_events_poll_count = either;
         platform->select_events_poll_count = either;
     }
@@ -170,9 +172,10 @@ static inline void sys_poll_events_with_poll(GlobalContext *glb, int timeout_ms)
     int fd_index;
     if (listeners_poll_count < 0 || select_events_poll_count < 0) {
         // Means it is dirty and should be rebuilt.
+        struct ListHead *select_events = synclist_wrlock(&glb->select_events);
         size_t select_events_new_count;
         if (select_events_poll_count < 0) {
-            select_event_count_and_destroy_closed(NULL, NULL, &select_events_new_count, glb);
+            select_event_count_and_destroy_closed(select_events, NULL, NULL, &select_events_new_count, glb);
         } else {
             select_events_new_count = select_events_poll_count;
         }
@@ -224,8 +227,7 @@ static inline void sys_poll_events_with_poll(GlobalContext *glb, int timeout_ms)
             synclist_unlock(&glb->listeners);
         }
 
-        struct ListHead *select_events = synclist_rdlock(&glb->select_events);
-        // We put listeners first
+        // We put select events next
         LIST_FOR_EACH (item, select_events) {
             struct SelectEvent *select_event = GET_LIST_ENTRY(item, struct SelectEvent, head);
             if (select_event->read || select_event->write) {