[glib] GDBusWorker: tolerate read errors while closing



commit 0f01bef4b458b73f2500ad8926b9c8a886215dc3
Author: Simon McVittie <simon mcvittie collabora co uk>
Date:   Fri Nov 11 14:41:50 2011 +0000

    GDBusWorker: tolerate read errors while closing
    
    My previous fix for GNOME#662100 was incomplete: it seems that with some
    timings, the stream can be closed with an async read in-flight. This
    can make the read fail immediately with G_IO_ERROR_CLOSED instead of
    becoming cancelled.
    
    This happens reliably on an embedded device, and rarely on my laptop;
    repeating the test 100 times in quick succession reliably reproduces
    the bug on my laptop.
    
    It seems as though what we really want is to ignore read errors, once
    we've established that we want to close the connection anyway - this
    means that after asking to close, you're immune to exit-on-close,
    which seems like a good rule.
    
    An additional subtlety is that continuing to read after we know we
    want to close is still required, otherwise we'll never emit ::closed.
    
    Bug: https://bugzilla.gnome.org/show_bug.cgi?id=662100
    Bug-NB: NB#287088
    Signed-off-by: Simon McVittie <simon mcvittie collabora co uk>
    Reviewed-by: Colin Walters <walters verbum org>

 gio/gdbusprivate.c |   22 +++++++++++++++++++++-
 1 files changed, 21 insertions(+), 1 deletions(-)
---
diff --git a/gio/gdbusprivate.c b/gio/gdbusprivate.c
index 3d4b1dc..55ac883 100644
--- a/gio/gdbusprivate.c
+++ b/gio/gdbusprivate.c
@@ -384,6 +384,8 @@ struct GDBusWorker
   GList                              *write_pending_flushes;
   /* list of CloseData, protected by write_lock */
   GList                              *pending_close_attempts;
+  /* no lock - only used from the worker thread */
+  gboolean                            close_expected;
 };
 
 static void _g_dbus_worker_unref (GDBusWorker *worker);
@@ -665,8 +667,18 @@ _g_dbus_worker_do_read_cb (GInputStream  *input_stream,
        * if the GDBusConnection tells us to close (either via
        * _g_dbus_worker_stop, which is called on last-unref, or directly),
        * so a cancelled read must mean our connection was closed locally.
+       *
+       * If we're closing, other errors are possible - notably,
+       * G_IO_ERROR_CLOSED can be seen if we close the stream with an async
+       * read in-flight. It seems sensible to treat all read errors during
+       * closing as an expected thing that doesn't trip exit-on-close.
+       *
+       * Because close_expected can't be set until we get into the worker
+       * thread, but the cancellable is signalled sooner (from another
+       * thread), we do still need to check the error.
        */
-      if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
+      if (worker->close_expected ||
+          g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
         _g_dbus_worker_emit_disconnected (worker, FALSE, NULL);
       else
         _g_dbus_worker_emit_disconnected (worker, TRUE, error);
@@ -806,6 +818,10 @@ _g_dbus_worker_do_read_cb (GInputStream  *input_stream,
 static void
 _g_dbus_worker_do_read_unlocked (GDBusWorker *worker)
 {
+  /* Note that we do need to keep trying to read even if close_expected is
+   * true, because only failing a read causes us to signal 'closed'.
+   */
+
   /* if bytes_wanted is zero, it means start reading a message */
   if (worker->read_buffer_bytes_wanted == 0)
     {
@@ -1396,6 +1412,7 @@ maybe_write_next_message (GDBusWorker *worker)
   /* if we want to close the connection, that takes precedence */
   if (worker->pending_close_attempts != NULL)
     {
+      worker->close_expected = TRUE;
       worker->output_pending = TRUE;
 
       g_io_stream_close_async (worker->stream, G_PRIORITY_DEFAULT,
@@ -1638,6 +1655,9 @@ _g_dbus_worker_close (GDBusWorker         *worker,
       (cancellable == NULL ? NULL : g_object_ref (cancellable));
   close_data->result = (result == NULL ? NULL : g_object_ref (result));
 
+  /* Don't set worker->close_expected here - we're in the wrong thread.
+   * It'll be set before the actual close happens.
+   */
   g_cancellable_cancel (worker->cancellable);
   schedule_write_in_worker_thread (worker, NULL, close_data);
 }



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]