SoupSessionSync: fix messages getting stuck forever

Messages in SoupSessionSync were getting stuck forever if they didn't
manage to get a connection on the first try; the test programs didn't
pick this up because none of them queue enough messages all at once to
hit the max-conns limit.

Fix the messages-getting-stuck bug, and add a test case to misc-test
to verify it. Also fix another SoupSessionSync bug where cancelling
several messages at once could cause spurious errors.

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

2 files changed, 129 insertions, 0 deletions

diff --git a/libsoup/soup-session-sync.c b/libsoup/soup-session-sync.c
index 408e3073..344f2343 100644
--- a/libsoup/soup-session-sync.c
+++ b/libsoup/soup-session-sync.c
@@ -298,6 +298,7 @@ process_queue_item (SoupMessageQueueItem *item)
 			item->state = SOUP_MESSAGE_FINISHED;
 			soup_message_finished (item->msg);
 			soup_session_unqueue_item (session, item);
+			g_cond_broadcast (priv->cond);
 			break;
 
 		default:
@@ -373,8 +374,10 @@ cancel_message (SoupSession *session, SoupMessage *msg, guint status_code)
 {
 	SoupSessionSyncPrivate *priv = SOUP_SESSION_SYNC_GET_PRIVATE (session);
 
+	g_mutex_lock (priv->lock);
 	SOUP_SESSION_CLASS (soup_session_sync_parent_class)->cancel_message (session, msg, status_code);
 	g_cond_broadcast (priv->cond);
+	g_mutex_unlock (priv->lock);
 }
 
 static void
diff --git a/tests/misc-test.c b/tests/misc-test.c
index 71263ee6..d3af982e 100644
--- a/tests/misc-test.c
+++ b/tests/misc-test.c
@@ -20,6 +20,7 @@
 
 SoupServer *server;
 SoupURI *base_uri;
+GMutex *server_mutex;
 
 static gboolean
 auth_callback (SoupAuthDomain *auth_domain, SoupMessage *msg,
@@ -102,6 +103,13 @@ server_callback (SoupServer *server, SoupMessage *msg,
 {
 	SoupURI *uri = soup_message_get_uri (msg);
 
+	/* The way this gets used in the tests, we don't actually
+	 * need to hold it through the whole function, so it's simpler
+	 * to just release it right away.
+	 */
+	g_mutex_lock (server_mutex);
+	g_mutex_unlock (server_mutex);
+
 	soup_message_headers_append (msg->response_headers,
 				     "X-Handled-By", "server_callback");
 
@@ -794,6 +802,121 @@ do_persistent_connection_timeout_test (void)
 	soup_test_session_abort_unref (session);
 }
 
+static GMainLoop *max_conns_loop;
+static int msgs_done;
+#define MAX_CONNS 2
+#define TEST_CONNS (MAX_CONNS * 2)
+
+static gboolean
+idle_start_server (gpointer data)
+{
+	g_mutex_unlock (server_mutex);
+	return FALSE;
+}
+
+static gboolean
+quit_loop (gpointer data)
+{
+	g_main_loop_quit (max_conns_loop);
+	return FALSE;
+}
+
+static void
+max_conns_request_started (SoupSession *session, SoupMessage *msg,
+			   SoupSocket *socket, gpointer user_data)
+{
+	if (++msgs_done == MAX_CONNS)
+		g_timeout_add (100, quit_loop, NULL);
+}
+
+static void
+max_conns_message_complete (SoupSession *session, SoupMessage *msg, gpointer user_data)
+{
+	if (++msgs_done == TEST_CONNS)
+		g_main_loop_quit (max_conns_loop);
+}
+
+static void
+do_max_conns_test_for_session (SoupSession *session)
+{
+	SoupMessage *msgs[TEST_CONNS];
+	int i;
+	guint timeout_id;
+
+	max_conns_loop = g_main_loop_new (NULL, TRUE);
+
+	g_mutex_lock (server_mutex);
+
+	g_signal_connect (session, "request-started",
+			  G_CALLBACK (max_conns_request_started), NULL);
+	msgs_done = 0;
+	for (i = 0; i < TEST_CONNS; i++) {
+		msgs[i] = soup_message_new_from_uri ("GET", base_uri);
+		g_object_ref (msgs[i]);
+		soup_session_queue_message (session, msgs[i],
+					    max_conns_message_complete, NULL);
+	}
+
+	g_main_loop_run (max_conns_loop);
+	if (msgs_done != MAX_CONNS) {
+		debug_printf (1, "  Queued %d connections out of max %d?",
+			      msgs_done, MAX_CONNS);
+		errors++;
+	}
+	g_signal_handlers_disconnect_by_func (session, max_conns_request_started, NULL);
+
+	msgs_done = 0;
+	g_idle_add (idle_start_server, NULL);
+	timeout_id = g_timeout_add (1000, quit_loop, NULL);
+	g_main_loop_run (max_conns_loop);
+
+	for (i = 0; i < TEST_CONNS; i++) {
+		if (!SOUP_STATUS_IS_SUCCESSFUL (msgs[i]->status_code)) {
+			debug_printf (1, "    Message %d failed? %d %s\n",
+				      i, msgs[i]->status_code,
+				      msgs[i]->reason_phrase ? msgs[i]->reason_phrase : "-");
+			errors++;
+		}
+	}
+
+	if (msgs_done != TEST_CONNS) {
+		/* Clean up so we don't get a spurious "Leaked
+		 * session" error.
+		 */
+		for (i = 0; i < TEST_CONNS; i++)
+			soup_session_cancel_message (session, msgs[i], SOUP_STATUS_CANCELLED);
+		g_main_loop_run (max_conns_loop);
+		g_source_remove (timeout_id);
+	}
+
+	g_main_loop_unref (max_conns_loop);
+
+	for (i = 0; i < TEST_CONNS; i++)
+		g_object_unref (msgs[i]);
+}
+
+static void
+do_max_conns_test (void)
+{
+	SoupSession *session;
+
+	debug_printf (1, "\nExceeding max-conns\n");
+
+	debug_printf (1, "  Async session\n");
+	session = soup_test_session_new (SOUP_TYPE_SESSION_ASYNC,
+					 SOUP_SESSION_MAX_CONNS, MAX_CONNS,
+					 NULL);
+	do_max_conns_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,
+					 SOUP_SESSION_MAX_CONNS, MAX_CONNS,
+					 NULL);
+	do_max_conns_test_for_session (session);
+	soup_test_session_abort_unref (session);
+}
+
 int
 main (int argc, char **argv)
 {
@@ -801,6 +924,8 @@ main (int argc, char **argv)
 
 	test_init (argc, argv, NULL);
 
+	server_mutex = g_mutex_new ();
+
 	server = soup_test_server_new (TRUE);
 	soup_server_add_handler (server, NULL, server_callback, NULL, NULL);
 	base_uri = soup_uri_new ("http://127.0.0.1/");
@@ -822,6 +947,7 @@ main (int argc, char **argv)
 	do_content_length_framing_test ();
 	do_accept_language_test ();
 	do_persistent_connection_timeout_test ();
+	do_max_conns_test ();
 
 	soup_uri_free (base_uri);
 	soup_test_server_quit_unref (server);