From: Dan Winship Date: Mon, 10 Jan 2011 18:47:52 +0000 (-0500) Subject: soup_session_cancel_message: fix up, especially in sync sessions X-Git-Tag: LIBSOUP_2_33_5~1 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=0f7ff260847a703ff90d77b892d6048e26cc768c;p=platform%2Fupstream%2Flibsoup.git 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 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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