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>

diff --git a/gio/gdbusprivate.c b/gio/gdbusprivate.c
index 3d4b1dc6185f45ad0a2e66ed61f96deca8345298..55ac883ce811b8ef9cbf25dd486bfaf1da2d15f7 100644 (file)
--- 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);
 }