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

6 files changed, 126 insertions, 18 deletions

diff --git a/libsoup/soup-message-io.c b/libsoup/soup-message-io.c
index 5a79d2fc..290d781b 100644
--- 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 4442cd2c..88ce313e 100644
--- 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 183ccbb3..e6751fa9 100644
--- 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 bd6aa6a1..fdc83ae4 100644
--- 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 d3af982e..411cb11e 100644
--- 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 b712f9f2..d3b62794 100644
--- 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