chiark / gitweb /
event: when unreffing an event source from its own handler, detach fd from epoll
authorLennart Poettering <lennart@poettering.net>
Fri, 13 Dec 2013 03:03:30 +0000 (04:03 +0100)
committerLennart Poettering <lennart@poettering.net>
Fri, 13 Dec 2013 03:06:43 +0000 (04:06 +0100)
The pattern of unreffing an IO event source and then closing its fd is
frequently seen in even source callbacks. Previously this likely
resultet in us removing the fd from the epoll after it was closed which
is problematic, since while we were dispatching we always kept an extra
reference to event source objects because we might still need it later.

TODO
src/libsystemd-bus/sd-event.c
src/libsystemd-bus/test-event.c

diff --git a/TODO b/TODO
index 4274b37..0ea4109 100644 (file)
--- a/TODO
+++ b/TODO
@@ -134,8 +134,7 @@ Features:
   - fix sd-event hookup when we connect to multiple servers one after the other
 
 * sd-event
-  - allow multiple signal handlers per signal
-  - when dispatching an event source then _unref() on it should remove it from the epoll
+  - allow multiple signal handlers per signal?
 
 * in the final killing spree, detect processes from the root directory, and
   complain loudly if they have argv[0][0] == '@' set.
index b396432..6af52ec 100644 (file)
@@ -58,6 +58,7 @@ struct sd_event_source {
         EventSourceType type:4;
         int enabled:3;
         bool pending:1;
+        bool dispatching:1;
 
         int priority;
         unsigned pending_index;
@@ -993,8 +994,21 @@ _public_ sd_event_source* sd_event_source_unref(sd_event_source *s) {
         assert(s->n_ref >= 1);
         s->n_ref--;
 
-        if (s->n_ref <= 0)
-                source_free(s);
+        if (s->n_ref <= 0) {
+                /* Here's a special hack: when we are called from a
+                 * dispatch handler we won't free the event source
+                 * immediately, but we will detach the fd from the
+                 * epoll. This way it is safe for the caller to unref
+                 * the event source and immediately close the fd, but
+                 * we still retain a valid event source object after
+                 * the callback. */
+
+                if (s->dispatching) {
+                        if (s->type == SOURCE_IO)
+                                source_io_unregister(s);
+                } else
+                        source_free(s);
+        }
 
         return NULL;
 }
@@ -1689,7 +1703,7 @@ static int source_dispatch(sd_event_source *s) {
                         return r;
         }
 
-        sd_event_source_ref(s);
+        s->dispatching = true;
 
         switch (s->type) {
 
@@ -1734,12 +1748,16 @@ static int source_dispatch(sd_event_source *s) {
                 break;
         }
 
-        if (r < 0) {
+        s->dispatching = false;
+
+        if (r < 0)
                 log_debug("Event source %p returned error, disabling: %s", s, strerror(-r));
+
+        if (s->n_ref == 0)
+                source_free(s);
+        else if (r < 0)
                 sd_event_source_set_enabled(s, SD_EVENT_OFF);
-        }
 
-        sd_event_source_unref(s);
         return 1;
 }
 
@@ -1761,10 +1779,18 @@ static int event_prepare(sd_event *e) {
                         return r;
 
                 assert(s->prepare);
+
+                s->dispatching = true;
                 r = s->prepare(s, s->userdata);
+                s->dispatching = false;
+
                 if (r < 0)
-                        return r;
+                        log_debug("Prepare callback of event source %p returned error, disabling: %s", s, strerror(-r));
 
+                if (s->n_ref == 0)
+                        source_free(s);
+                else if (r < 0)
+                        sd_event_source_set_enabled(s, SD_EVENT_OFF);
         }
 
         return 0;
index 5317008..28ef6a3 100644 (file)
@@ -28,9 +28,15 @@ static int prepare_handler(sd_event_source *s, void *userdata) {
         return 1;
 }
 
-static bool got_a, got_b, got_c;
+static bool got_a, got_b, got_c, got_unref;
 static unsigned got_d;
 
+static int unref_handler(sd_event_source *s, int fd, uint32_t revents, void *userdata) {
+        sd_event_source_unref(s);
+        got_unref = true;
+        return 0;
+}
+
 static int io_handler(sd_event_source *s, int fd, uint32_t revents, void *userdata) {
 
         log_info("got IO on %c", PTR_TO_INT(userdata));
@@ -155,18 +161,26 @@ static int exit_handler(sd_event_source *s, void *userdata) {
 
 int main(int argc, char *argv[]) {
         sd_event *e = NULL;
-        sd_event_source *w = NULL, *x = NULL, *y = NULL, *z = NULL, *q = NULL;
+        sd_event_source *w = NULL, *x = NULL, *y = NULL, *z = NULL, *q = NULL, *t = NULL;
         static const char ch = 'x';
-        int a[2] = { -1, -1 }, b[2] = { -1, -1}, d[2] = { -1, -1};
+        int a[2] = { -1, -1 }, b[2] = { -1, -1}, d[2] = { -1, -1}, k[2] = { -1, -1 };
 
         assert_se(pipe(a) >= 0);
         assert_se(pipe(b) >= 0);
         assert_se(pipe(d) >= 0);
+        assert_se(pipe(k) >= 0);
 
         assert_se(sd_event_default(&e) >= 0);
 
         assert_se(sd_event_set_watchdog(e, true) >= 0);
 
+        /* Test whether we cleanly can destroy an io event source from its own handler */
+        got_unref = false;
+        assert_se(sd_event_add_io(e, k[0], EPOLLIN, unref_handler, NULL, &t) >= 0);
+        assert_se(write(k[1], &ch, 1) == 1);
+        assert_se(sd_event_run(e, (uint64_t) -1) >= 1);
+        assert_se(got_unref);
+
         got_a = false, got_b = false, got_c = false, got_d = 0;
 
         /* Add a oneshot handler, trigger it, re-enable it, and trigger
@@ -227,6 +241,8 @@ int main(int argc, char *argv[]) {
 
         close_pipe(a);
         close_pipe(b);
+        close_pipe(d);
+        close_pipe(k);
 
         return 0;
 }