soup_session_cancel_message: fix up, especially in sync sessions

Cancelling a message from another thread had some race conditions that
could sometimes cause crashes. Fix things up a bit by using
GCancellable to interrupt the I/O, rather than calling
soup_message_io_finished() directly.

Also added a test for this case to tests/misc-test, although
unfortunately due to the raciness of the bug, it only failed
sporadically even before the fix (but seems to fail never now).

https://bugzilla.gnome.org/show_bug.cgi?id=637741

diff --git a/libsoup/soup-message-io.c b/libsoup/soup-message-io.c
index 5a79d2f..290d781 100644 (file)
--- a/libsoup/soup-message-io.c
+++ b/libsoup/soup-message-io.c
@@ -47,6 +47,7 @@ typedef struct {
        SoupSocket           *sock;
        SoupMessageQueueItem *item;
        SoupMessageIOMode     mode;
+       GCancellable         *cancellable;
 
        SoupMessageIOState    read_state;
        SoupEncoding          read_encoding;
@@ -284,7 +285,7 @@ read_metadata (SoupMessage *msg, gboolean to_blank)
                status = soup_socket_read_until (io->sock, read_buf,
                                                 sizeof (read_buf),
                                                 "\n", 1, &nread, &got_lf,
-                                                NULL, &error);
+                                                io->cancellable, &error);
                switch (status) {
                case SOUP_SOCKET_OK:
                        g_byte_array_append (io->read_meta_buf, read_buf, nread);
@@ -461,7 +462,7 @@ read_body_chunk (SoupMessage *msg)
 
                status = soup_socket_read (io->sock,
                                           (guchar *)buffer->data, len,
-                                          &nread, NULL, &error);
+                                          &nread, io->cancellable, &error);
 
                if (status == SOUP_SOCKET_OK && nread) {
                        buffer->length = nread;
@@ -531,7 +532,8 @@ write_data (SoupMessage *msg, const char *data, guint len, gboolean body)
                status = soup_socket_write (io->sock,
                                            data + io->written,
                                            len - io->written,
-                                           &nwrote, NULL, &error);
+                                           &nwrote,
+                                           io->cancellable, &error);
                switch (status) {
                case SOUP_SOCKET_EOF:
                case SOUP_SOCKET_ERROR:
@@ -1124,6 +1126,7 @@ soup_message_io_client (SoupMessageQueueItem *item,
 
        io->item = item;
        soup_message_queue_item_ref (item);
+       io->cancellable = item->cancellable;
 
        io->read_body       = item->msg->response_body;
        io->write_body      = item->msg->request_body;

diff --git a/libsoup/soup-message-queue.c b/libsoup/soup-message-queue.c
index 4442cd2..88ce313 100644 (file)
--- a/libsoup/soup-message-queue.c
+++ b/libsoup/soup-message-queue.c
@@ -79,6 +79,8 @@ queue_message_restarted (SoupMessage *msg, gpointer user_data)
                item->conn = NULL;
        }
 
+       g_cancellable_reset (item->cancellable);
+
        item->state = SOUP_MESSAGE_STARTING;
 }
 

diff --git a/libsoup/soup-session-async.c b/libsoup/soup-session-async.c
index 183ccbb..e6751fa 100644 (file)
--- a/libsoup/soup-session-async.c
+++ b/libsoup/soup-session-async.c
@@ -501,14 +501,22 @@ cancel_message (SoupSession *session, SoupMessage *msg,
 
        queue = soup_session_get_queue (session);
        item = soup_message_queue_lookup (queue, msg);
-       if (!item || item->state != SOUP_MESSAGE_FINISHING)
+       if (!item)
                return;
 
        /* Force it to finish immediately, so that
         * soup_session_abort (session); g_object_unref (session);
-        * will work.
+        * will work. (The soup_session_cancel_message() docs
+        * point out that the callback will be invoked from
+        * within the cancel call.)
         */
-       process_queue_item (item, &dummy, FALSE);
+       if (soup_message_io_in_progress (msg))
+               soup_message_io_finished (msg);
+       else if (item->state != SOUP_MESSAGE_FINISHED)
+               item->state = SOUP_MESSAGE_FINISHING;
+
+       if (item->state != SOUP_MESSAGE_FINISHED)
+               process_queue_item (item, &dummy, FALSE);
 
        soup_message_queue_item_unref (item);
 }

diff --git a/libsoup/soup-session.c b/libsoup/soup-session.c
index bd6aa6a..fdc83ae 100644 (file)
--- a/libsoup/soup-session.c
+++ b/libsoup/soup-session.c
@@ -1633,14 +1633,8 @@ cancel_message (SoupSession *session, SoupMessage *msg, guint status_code)
        item = soup_message_queue_lookup (priv->queue, msg);
        g_return_if_fail (item != NULL);
 
-       if (item->cancellable)
-               g_cancellable_cancel (item->cancellable);
-
        soup_message_set_status (msg, status_code);
-       if (soup_message_io_in_progress (msg))
-               soup_message_io_finished (msg);
-       else
-               item->state = SOUP_MESSAGE_FINISHING;
+       g_cancellable_cancel (item->cancellable);
 
        soup_message_queue_item_unref (item);
 }

diff --git a/tests/misc-test.c b/tests/misc-test.c
index d3af982..411cb11 100644 (file)
--- a/tests/misc-test.c
+++ b/tests/misc-test.c
@@ -96,6 +96,15 @@ setup_timeout_persistent (SoupServer *server, SoupSocket *sock)
                          G_CALLBACK (timeout_request_started), NULL);
 }
 
+static gboolean
+timeout_finish_message (gpointer msg)
+{
+       SoupServer *server = g_object_get_data (G_OBJECT (msg), "server");
+
+       soup_server_unpause_message (server, msg);
+       return FALSE;
+}
+
 static void
 server_callback (SoupServer *server, SoupMessage *msg,
                 const char *path, GHashTable *query,
@@ -178,6 +187,13 @@ server_callback (SoupServer *server, SoupMessage *msg,
                setup_timeout_persistent (server, sock);
        }
 
+       if (!strcmp (path, "/slow")) {
+               soup_server_pause_message (server, msg);
+               g_object_set_data (G_OBJECT (msg), "server", server);
+               soup_add_timeout (soup_server_get_async_context (server),
+                                 1000, timeout_finish_message, msg);
+       }
+
        soup_message_set_status (msg, SOUP_STATUS_OK);
        if (!strcmp (uri->host, "foo")) {
                soup_message_set_response (msg, "text/plain",
@@ -917,6 +933,79 @@ do_max_conns_test (void)
        soup_test_session_abort_unref (session);
 }
 
+static gboolean
+cancel_message_timeout (gpointer msg)
+{
+       SoupSession *session = g_object_get_data (G_OBJECT (msg), "session");
+
+       soup_session_cancel_message (session, msg, SOUP_STATUS_CANCELLED);
+       g_object_unref (msg);
+       g_object_unref (session);
+       return FALSE;
+}
+
+static gpointer
+cancel_message_thread (gpointer msg)
+{
+       SoupSession *session = g_object_get_data (G_OBJECT (msg), "session");
+
+       g_usleep (100000); /* .1s */
+       soup_session_cancel_message (session, msg, SOUP_STATUS_CANCELLED);
+       g_object_unref (msg);
+       g_object_unref (session);
+       return NULL;
+}
+
+static void
+do_cancel_while_reading_test_for_session (SoupSession *session)
+{
+       SoupMessage *msg;
+       GThread *thread = NULL;
+       SoupURI *uri;
+
+       uri = soup_uri_new_with_base (base_uri, "/slow");
+       msg = soup_message_new_from_uri ("GET", uri);
+       soup_uri_free (uri);
+
+       g_object_set_data (G_OBJECT (msg), "session", session);
+       g_object_ref (msg);
+       g_object_ref (session);
+       if (SOUP_IS_SESSION_ASYNC (session))
+               g_timeout_add (100, cancel_message_timeout, msg);
+       else
+               thread = g_thread_create (cancel_message_thread, msg, TRUE, NULL);
+
+       soup_session_send_message (session, msg);
+
+       if (msg->status_code != SOUP_STATUS_CANCELLED) {
+               debug_printf (1, "      FAILED: %d %s (expected Cancelled)\n",
+                             msg->status_code, msg->reason_phrase);
+               errors++;
+       }
+       g_object_unref (msg);
+
+       if (thread)
+               g_thread_join (thread);
+}
+
+static void
+do_cancel_while_reading_test (void)
+{
+       SoupSession *session;
+
+       debug_printf (1, "\nCancelling message while reading response\n");
+
+       debug_printf (1, "  Async session\n");
+       session = soup_test_session_new (SOUP_TYPE_SESSION_ASYNC, NULL);
+       do_cancel_while_reading_test_for_session (session);
+       soup_test_session_abort_unref (session);
+
+       debug_printf (1, "  Sync session\n");
+       session = soup_test_session_new (SOUP_TYPE_SESSION_SYNC, NULL);
+       do_cancel_while_reading_test_for_session (session);
+       soup_test_session_abort_unref (session);
+}
+
 int
 main (int argc, char **argv)
 {
@@ -948,6 +1037,7 @@ main (int argc, char **argv)
        do_accept_language_test ();
        do_persistent_connection_timeout_test ();
        do_max_conns_test ();
+       do_cancel_while_reading_test ();
 
        soup_uri_free (base_uri);
        soup_test_server_quit_unref (server);

diff --git a/tests/timeout-test.c b/tests/timeout-test.c
index b712f9f..d3b6279 100644 (file)
--- a/tests/timeout-test.c
+++ b/tests/timeout-test.c
@@ -137,6 +137,15 @@ do_timeout_tests (char *fast_uri, char *slow_uri)
        soup_test_session_abort_unref (timeout_session);
 }
 
+static gboolean
+timeout_finish_message (gpointer msg)
+{
+       SoupServer *server = g_object_get_data (G_OBJECT (msg), "server");
+
+       soup_server_unpause_message (server, msg);
+       return FALSE;
+}
+
 static void
 server_handler (SoupServer        *server,
                SoupMessage       *msg, 
@@ -145,15 +154,17 @@ server_handler (SoupServer        *server,
                SoupClientContext *client,
                gpointer           user_data)
 {
-       if (!strcmp (path, "/slow")) {
-               /* Sleep 1.1 seconds. */
-               g_usleep (1100000);
-       }
-
        soup_message_set_status (msg, SOUP_STATUS_OK);
        soup_message_set_response (msg, "text/plain",
                                   SOUP_MEMORY_STATIC,
                                   "ok\r\n", 4);
+
+       if (!strcmp (path, "/slow")) {
+               soup_server_pause_message (server, msg);
+               g_object_set_data (G_OBJECT (msg), "server", server);
+               soup_add_timeout (soup_server_get_async_context (server),
+                                 1100, timeout_finish_message, msg);
+       }
 }
 
 int