From 0f01bef4b458b73f2500ad8926b9c8a886215dc3 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Fri, 11 Nov 2011 14:41:50 +0000 Subject: [PATCH] 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 Reviewed-by: Colin Walters --- gio/gdbusprivate.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/gio/gdbusprivate.c b/gio/gdbusprivate.c index 3d4b1dc61..55ac883ce 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); }