GNOME.org
 

libsoup r1168 - in trunk: . libsoup tests

Author: danw
Date: Tue Sep 30 15:43:17 2008
New Revision: 1168
URL: http://svn.gnome.org/viewvc/libsoup?rev=1168&view=rev

Log:
	* libsoup/soup-session-async.c (do_idle_run_queue): store the
	GSource in priv, don't ref the session. Otherwise the session
	won't get destroyed if you abort it and then don't return to its
	main loop. (addendum to #498509, Arnout Vandecappelle)
	(finalize): Destroy the idle_run_queue source when finalizing.
	(run_queue, got_connection): Ref the session when calling
	soup_connection_connect_async(), and do a
	do_idle_run_queue()+unref in got_connection, to ensure correct
	handling regardless of what the application does with its own ref
	on the session.
	(final_finished): Likewise, ref/do_idle_run_queue/unref rather
	than calling run_queue directly and playing with weak pointers.

	* libsoup/soup-session.c (connect_result): ref the session around
	the cancel-if-error loop

	Fixes #533473, crash in seahorse when connecting to a
	non-responsive key server.

	* tests/misc-test.c (do_callback_unref_test): Add a test for the
	bug in #533473.

	* tests/test-utils.c (soup_test_session_abort_unref): abort and
	unref a SoupSession, and consider it an error if the session still
	exists afterward. Suggested by Arnout Vandecappelle.
	(test_server_shutdown): Likewise, consider it an error if the
	server is leaked.

	* tests/*.c: Use soup_test_session_abort_unref().


Modified:
   trunk/ChangeLog
   trunk/libsoup/soup-session-async.c
   trunk/libsoup/soup-session.c
   trunk/tests/auth-test.c
   trunk/tests/chunk-test.c
   trunk/tests/context-test.c
   trunk/tests/continue-test.c
   trunk/tests/misc-test.c
   trunk/tests/ntlm-test.c
   trunk/tests/pull-api.c
   trunk/tests/redirect-test.c
   trunk/tests/test-utils.c
   trunk/tests/test-utils.h
   trunk/tests/xmlrpc-test.c

Modified: trunk/libsoup/soup-session-async.c
==============================================================================
--- trunk/libsoup/soup-session-async.c	(original)
+++ trunk/libsoup/soup-session-async.c	Tue Sep 30 15:43:17 2008
@@ -24,7 +24,7 @@
  **/
 
 static gboolean run_queue (SoupSessionAsync *sa);
-static void do_idle_run_queue (SoupSession *session);
+static void do_idle_run_queue (SoupSessionAsync *sa);
 
 static void  queue_message   (SoupSession *session, SoupMessage *req,
 			      SoupSessionCallback callback, gpointer user_data);
@@ -32,19 +32,41 @@
 
 G_DEFINE_TYPE (SoupSessionAsync, soup_session_async, SOUP_TYPE_SESSION)
 
+typedef struct {
+	GSource *idle_run_queue_source;
+} SoupSessionAsyncPrivate;
+#define SOUP_SESSION_ASYNC_GET_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE ((o), SOUP_TYPE_SESSION_ASYNC, SoupSessionAsyncPrivate))
+
 static void
 soup_session_async_init (SoupSessionAsync *sa)
 {
 }
 
 static void
+finalize (GObject *object)
+{
+	SoupSessionAsyncPrivate *priv = SOUP_SESSION_ASYNC_GET_PRIVATE (object);
+
+	if (priv->idle_run_queue_source)
+		g_source_destroy (priv->idle_run_queue_source);
+
+	G_OBJECT_CLASS (soup_session_async_parent_class)->finalize (object);
+}
+
+static void
 soup_session_async_class_init (SoupSessionAsyncClass *soup_session_async_class)
 {
 	SoupSessionClass *session_class = SOUP_SESSION_CLASS (soup_session_async_class);
+	GObjectClass *object_class = G_OBJECT_CLASS (session_class);
+
+	g_type_class_add_private (soup_session_async_class,
+				  sizeof (SoupSessionAsyncPrivate));
 
 	/* virtual method override */
 	session_class->queue_message = queue_message;
 	session_class->send_message = send_message;
+
+	object_class->finalize = finalize;
 }
 
 
@@ -86,12 +108,12 @@
 
 
 static void
-connection_closed (SoupConnection *conn, gpointer session)
+connection_closed (SoupConnection *conn, gpointer sa)
 {
 	/* Run the queue in case anyone was waiting for a connection
 	 * to be closed.
 	 */
-	do_idle_run_queue (session);
+	do_idle_run_queue (sa);
 }
 
 static void
@@ -99,34 +121,29 @@
 {
 	SoupSessionAsync *sa = user_data;
 
-	if (status != SOUP_STATUS_OK) {
-		/* The connection attempt failed, and thus @conn was
-		 * closed and the open connection count for the
-		 * session has been decremented. (If the failure was
-		 * fatal, then SoupSession itself will have dealt
-		 * with cancelling any pending messages for that
-		 * host, so we don't need to worry about that here.)
-		 * However, there may be other messages in the
-		 * queue that were waiting for the connection count
-		 * to go down, so run the queue now.
+	if (status == SOUP_STATUS_OK) {
+		g_signal_connect (conn, "disconnected",
+				  G_CALLBACK (connection_closed), sa);
+
+		/* @conn has been marked reserved by SoupSession, but
+		 * we don't actually have any specific message in mind
+		 * for it. (In particular, the message we were
+		 * originally planning to queue on it may have already
+		 * been queued on some other connection that became
+		 * available while we were waiting for this one to
+		 * connect.) So we release the connection into the
+		 * idle pool and then just run the queue and see what
+		 * happens.
 		 */
-		run_queue (sa);
-		return;
+		soup_connection_release (conn);
 	}
 
-	g_signal_connect (conn, "disconnected",
-			  G_CALLBACK (connection_closed), sa);
-
-	/* @conn has been marked reserved by SoupSession, but we don't
-	 * actually have any specific message in mind for it. (In
-	 * particular, the message we were originally planning to
-	 * queue on it may have already been queued on some other
-	 * connection that became available while we were waiting for
-	 * this one to connect.) So we release the connection into the
-	 * idle pool and then just run the queue and see what happens.
+	/* Even if the connection failed, we run the queue, since
+	 * there may have been messages waiting for the connection
+	 * count to go down.
 	 */
-	soup_connection_release (conn);
-	run_queue (sa);
+	do_idle_run_queue (sa);
+	g_object_unref (sa);
 }
 
 static gboolean
@@ -158,7 +175,7 @@
 
 		if (is_new) {
 			soup_connection_connect_async (conn, got_connection,
-						       session);
+						       g_object_ref (session));
 		} else
 			soup_connection_send_request (conn, msg);
 	}
@@ -195,46 +212,42 @@
 	SoupSessionAsyncQueueData *saqd = user_data;
 	SoupSessionAsync *sa = saqd->sa;
 
-	g_object_add_weak_pointer (G_OBJECT (sa), (gpointer)&sa);
-
+	g_object_ref (sa);
 	if (!SOUP_MESSAGE_IS_STARTING (req)) {
 		g_signal_handlers_disconnect_by_func (req, final_finished, saqd);
 		if (saqd->callback) {
 			saqd->callback ((SoupSession *)sa, req,
 					saqd->callback_data);
-			/* callback might destroy sa */
 		}
 
 		g_object_unref (req);
 		g_slice_free (SoupSessionAsyncQueueData, saqd);
 	}
 
-	if (sa) {
-		g_object_remove_weak_pointer (G_OBJECT (sa), (gpointer)&sa);
-		run_queue (sa);
-	}
+	do_idle_run_queue (sa);
+	g_object_unref (sa);
 }
 
 static gboolean
-idle_run_queue (gpointer user_data)
+idle_run_queue (gpointer sa)
 {
-	SoupSessionAsync *sa = user_data;
+	SoupSessionAsyncPrivate *priv = SOUP_SESSION_ASYNC_GET_PRIVATE (sa);
 
-	g_object_add_weak_pointer (G_OBJECT (sa), (gpointer)&sa);
-	g_object_unref (sa);
-
-	if (sa) {
-		g_object_remove_weak_pointer (G_OBJECT (sa), (gpointer)&sa);
-		run_queue (sa);
-	}
+	priv->idle_run_queue_source = NULL;
+	run_queue (sa);
 	return FALSE;
 }
 
 static void
-do_idle_run_queue (SoupSession *session)
+do_idle_run_queue (SoupSessionAsync *sa)
 {
-	soup_add_completion (soup_session_get_async_context (session),
-			     idle_run_queue, g_object_ref (session));
+	SoupSessionAsyncPrivate *priv = SOUP_SESSION_ASYNC_GET_PRIVATE (sa);
+
+	if (!priv->idle_run_queue_source) {
+		priv->idle_run_queue_source = soup_add_completion (
+			soup_session_get_async_context ((SoupSession *)sa),
+			idle_run_queue, sa);
+	}
 }
 
 static void
@@ -255,7 +268,7 @@
 				G_CALLBACK (final_finished), saqd);
 
 	SOUP_SESSION_CLASS (soup_session_async_parent_class)->queue_message (session, req, callback, user_data);
-	do_idle_run_queue (session);
+	do_idle_run_queue (sa);
 }
 
 static guint

Modified: trunk/libsoup/soup-session.c
==============================================================================
--- trunk/libsoup/soup-session.c	(original)
+++ trunk/libsoup/soup-session.c	Tue Sep 30 15:43:17 2008
@@ -960,6 +960,7 @@
 	 * any messages waiting for this host, since they're out
 	 * of luck.
 	 */
+	g_object_ref (session);
 	for (msg = soup_message_queue_first (priv->queue, &iter); msg; msg = soup_message_queue_next (priv->queue, &iter)) {
 		if (get_host_for_message (session, msg) == host) {
 			if (status == SOUP_STATUS_TRY_AGAIN) {
@@ -971,6 +972,7 @@
 			}
 		}
 	}
+	g_object_unref (session);
 }
 
 /**

Modified: trunk/tests/auth-test.c
==============================================================================
--- trunk/tests/auth-test.c	(original)
+++ trunk/tests/auth-test.c	Tue Sep 30 15:43:17 2008
@@ -503,8 +503,7 @@
 		errors++;
 	}
 
-	soup_session_abort (session);
-	g_object_unref (session);
+	soup_test_session_abort_unref (session);
 
 	g_object_unref (msg1);
 	g_object_unref (msg3);
@@ -537,8 +536,7 @@
 	g_main_loop_run (loop);
 	g_signal_handler_disconnect (session, auth_id);
 
-	soup_session_abort (session);
-	g_object_unref (session);
+	soup_test_session_abort_unref (session);
 
 	g_object_unref (msg1);
 
@@ -607,8 +605,7 @@
 
 		g_object_unref (msg);
 	}
-	soup_session_abort (session);
-	g_object_unref (session);
+	soup_test_session_abort_unref (session);
 
 	/* And now for some regression tests */
 	loop = g_main_loop_new (NULL, TRUE);
@@ -633,8 +630,7 @@
 	g_free (uri);
 
 	g_main_loop_run (loop);
-	soup_session_abort (session);
-	g_object_unref (session);
+	soup_test_session_abort_unref (session);
 
 	debug_printf (1, "\nTesting digest nonce expiration:\n");
 
@@ -726,8 +722,7 @@
 	do_digest_nonce_test (session, "Fourth", uri, FALSE, FALSE);
 	g_free (uri);
 
-	soup_session_abort (session);
-	g_object_unref (session);
+	soup_test_session_abort_unref (session);
 
 	/* Async auth */
 	do_async_auth_test (base_uri);

Modified: trunk/tests/chunk-test.c
==============================================================================
--- trunk/tests/chunk-test.c	(original)
+++ trunk/tests/chunk-test.c	Tue Sep 30 15:43:17 2008
@@ -248,8 +248,7 @@
 	do_request_test (session, base_uri);
 	debug_printf (2, "\n\n");
 	do_response_test (session, base_uri);
-	soup_session_abort (session);
-	g_object_unref (session);
+	soup_test_session_abort_unref (session);
 }
 
 static void

Modified: trunk/tests/context-test.c
==============================================================================
--- trunk/tests/context-test.c	(original)
+++ trunk/tests/context-test.c	Tue Sep 30 15:43:17 2008
@@ -194,8 +194,7 @@
 	}
 	g_object_unref (msg);
 
-	soup_session_abort (session);
-	g_object_unref (session);
+	soup_test_session_abort_unref (session);
 	g_free (uri);
 
 	g_cond_signal (test1_cond);
@@ -240,8 +239,7 @@
 	}
 	g_object_unref (msg);
 
-	soup_session_abort (session);
-	g_object_unref (session);
+	soup_test_session_abort_unref (session);
 	g_free (uri);
 
 	g_source_remove (idle);

Modified: trunk/tests/continue-test.c
==============================================================================
--- trunk/tests/continue-test.c	(original)
+++ trunk/tests/continue-test.c	Tue Sep 30 15:43:17 2008
@@ -109,8 +109,7 @@
 	events = NULL;
 	session = soup_test_session_new (SOUP_TYPE_SESSION_ASYNC, NULL);
 	soup_session_send_message (session, msg);
-	soup_session_abort (session);
-	g_object_unref (session);
+	soup_test_session_abort_unref (session);
 
 	va_start (ap, auth);
 	while ((expected_event = va_arg (ap, const char *))) {

Modified: trunk/tests/misc-test.c
==============================================================================
--- trunk/tests/misc-test.c	(original)
+++ trunk/tests/misc-test.c	Tue Sep 30 15:43:17 2008
@@ -46,8 +46,8 @@
 
 /* Host header handling: client must be able to override the default
  * value, server must be able to recognize different Host values.
+ * #539803.
  */
-
 static void
 do_host_test (void)
 {
@@ -65,8 +65,7 @@
 	soup_session_send_message (session, one);
 	soup_session_send_message (session, two);
 
-	soup_session_abort (session);
-	g_object_unref (session);
+	soup_test_session_abort_unref (session);
 
 	if (!SOUP_STATUS_IS_SUCCESSFUL (one->status_code)) {
 		debug_printf (1, "  Message 1 failed: %d %s\n",
@@ -91,6 +90,96 @@
 	g_object_unref (two);
 }
 
+/* Dropping the application's ref on the session from a callback
+ * should not cause the session to be freed at an incorrect time.
+ * (This test will crash if it fails.) #533473
+ */
+static void
+cu_one_completed (SoupSession *session, SoupMessage *msg, gpointer loop)
+{
+	debug_printf (2, "  Message 1 completed\n");
+	if (msg->status_code != SOUP_STATUS_CANT_CONNECT) {
+		debug_printf (1, "  Unexpected status on Message 1: %d %s\n",
+			      msg->status_code, msg->reason_phrase);
+		errors++;
+	}
+	g_object_unref (session);
+}
+
+static gboolean
+cu_idle_quit (gpointer loop)
+{
+	g_main_loop_quit (loop);
+	return FALSE;
+}
+
+static void
+cu_two_completed (SoupSession *session, SoupMessage *msg, gpointer loop)
+{
+	debug_printf (2, "  Message 2 completed\n");
+	if (msg->status_code != SOUP_STATUS_CANT_CONNECT) {
+		debug_printf (1, "  Unexpected status on Message 2: %d %s\n",
+			      msg->status_code, msg->reason_phrase);
+		errors++;
+	}
+	g_idle_add (cu_idle_quit, loop); 
+}
+
+static void
+do_callback_unref_test (void)
+{
+	SoupServer *bad_server;
+	SoupSession *session;
+	SoupMessage *one, *two;
+	GMainLoop *loop;
+	char *bad_uri;
+
+	debug_printf (1, "Callback unref handling\n");
+
+	/* Get a guaranteed-bad URI */
+	bad_server = soup_server_new (NULL, NULL);
+	bad_uri = g_strdup_printf ("http://localhost:%u/";,
+				   soup_server_get_port (bad_server));
+	g_object_unref (bad_server);
+
+	session = soup_test_session_new (SOUP_TYPE_SESSION_ASYNC, NULL);
+	g_object_add_weak_pointer (G_OBJECT (session), (gpointer *)&session);
+
+	loop = g_main_loop_new (NULL, TRUE);
+
+	one = soup_message_new ("GET", bad_uri);
+	g_object_add_weak_pointer (G_OBJECT (one), (gpointer *)&one);
+	two = soup_message_new ("GET", bad_uri);
+	g_object_add_weak_pointer (G_OBJECT (two), (gpointer *)&two);
+
+	soup_session_queue_message (session, one, cu_one_completed, loop);
+	soup_session_queue_message (session, two, cu_two_completed, loop);
+
+	g_main_loop_run (loop);
+	g_main_loop_unref (loop);
+
+	if (session) {
+		g_object_remove_weak_pointer (G_OBJECT (session), (gpointer *)&session);
+		debug_printf (1, "  Session not destroyed?\n");
+		errors++;
+		g_object_unref (session);
+	}
+	if (one) {
+		g_object_remove_weak_pointer (G_OBJECT (one), (gpointer *)&one);
+		debug_printf (1, "  Message 1 not destroyed?\n");
+		errors++;
+		g_object_unref (one);
+	}
+	if (two) {
+		g_object_remove_weak_pointer (G_OBJECT (two), (gpointer *)&two);
+		debug_printf (1, "  Message 2 not destroyed?\n");
+		errors++;
+		g_object_unref (two);
+	}
+
+	/* Otherwise, if we haven't crashed, we're ok. */
+}
+
 int
 main (int argc, char **argv)
 {
@@ -104,6 +193,7 @@
 				    soup_server_get_port (server));
 
 	do_host_test ();
+	do_callback_unref_test ();
 
 	g_free (base_uri);
 

Modified: trunk/tests/ntlm-test.c
==============================================================================
--- trunk/tests/ntlm-test.c	(original)
+++ trunk/tests/ntlm-test.c	Tue Sep 30 15:43:17 2008
@@ -385,8 +385,7 @@
 		    user != NULL ? SOUP_STATUS_OK :
 		    SOUP_STATUS_UNAUTHORIZED);
 
-	soup_session_abort (session);
-	g_object_unref (session);
+	soup_test_session_abort_unref (session);
 }
 
 static void

Modified: trunk/tests/pull-api.c
==============================================================================
--- trunk/tests/pull-api.c	(original)
+++ trunk/tests/pull-api.c	Tue Sep 30 15:43:17 2008
@@ -41,8 +41,7 @@
 	correct_response = soup_message_body_flatten (msg->response_body);
 
 	g_object_unref (msg);
-	soup_session_abort (session);
-	g_object_unref (session);
+	soup_test_session_abort_unref (session);
 }
 
 /* Pull API version 1: fully-async. More like a "poke" API. Rather
@@ -508,8 +507,7 @@
 			     TRUE, SOUP_STATUS_UNAUTHORIZED);
 	do_fully_async_test (session, base_uri, "/Basic/realm2/",
 			     TRUE, SOUP_STATUS_OK);
-	soup_session_abort (session);
-	g_object_unref (session);
+	soup_test_session_abort_unref (session);
 
 	debug_printf (1, "\nFully async, slow requests\n");
 	session = soup_test_session_new (SOUP_TYPE_SESSION_ASYNC, NULL);
@@ -521,8 +519,7 @@
 			     FALSE, SOUP_STATUS_UNAUTHORIZED);
 	do_fully_async_test (session, base_uri, "/Basic/realm2/",
 			     FALSE, SOUP_STATUS_OK);
-	soup_session_abort (session);
-	g_object_unref (session);
+	soup_test_session_abort_unref (session);
 
 	debug_printf (1, "\nSynchronously async\n");
 	session = soup_test_session_new (SOUP_TYPE_SESSION_ASYNC, NULL);
@@ -534,9 +531,7 @@
 				     SOUP_STATUS_UNAUTHORIZED);
 	do_synchronously_async_test (session, base_uri, "/Basic/realm2/",
 				     SOUP_STATUS_OK);
-
-	soup_session_abort (session);
-	g_object_unref (session);
+	soup_test_session_abort_unref (session);
 
 	soup_buffer_free (correct_response);
 

Modified: trunk/tests/redirect-test.c
==============================================================================
--- trunk/tests/redirect-test.c	(original)
+++ trunk/tests/redirect-test.c	Tue Sep 30 15:43:17 2008
@@ -189,15 +189,13 @@
 	debug_printf (1, "Async session\n");
 	for (n = 0; n < n_tests; n++)
 		do_test (session, base_uri, n);
-	soup_session_abort (session);
-	g_object_unref (session);
+	soup_test_session_abort_unref (session);
 
 	session = soup_test_session_new (SOUP_TYPE_SESSION_SYNC, NULL);
 	debug_printf (1, "Sync session\n");
 	for (n = 0; n < n_tests; n++)
 		do_test (session, base_uri, n);
-	soup_session_abort (session);
-	g_object_unref (session);
+	soup_test_session_abort_unref (session);
 }
 
 static void

Modified: trunk/tests/test-utils.c
==============================================================================
--- trunk/tests/test-utils.c	(original)
+++ trunk/tests/test-utils.c	Tue Sep 30 15:43:17 2008
@@ -100,14 +100,6 @@
 void
 test_cleanup (void)
 {
-	debug_printf (1, "\n");
-	if (errors) {
-		printf ("%s: %d error(s).%s\n",
-			g_get_prgname (), errors,
-			debug_level == 0 ? " Run with '-d' for details" : "");
-	} else
-		printf ("%s: OK\n", g_get_prgname ());
-
 #ifdef HAVE_APACHE
 	if (apache_running)
 		apache_cleanup ();
@@ -119,6 +111,14 @@
 		g_object_unref (logger);
 
 	g_main_context_unref (g_main_context_default ());
+
+	debug_printf (1, "\n");
+	if (errors) {
+		printf ("%s: %d error(s).%s\n",
+			g_get_prgname (), errors,
+			debug_level == 0 ? " Run with '-d' for details" : "");
+	} else
+		printf ("%s: OK\n", g_get_prgname ());
 }
 
 void
@@ -225,6 +225,21 @@
 	return session;
 }
 
+void
+soup_test_session_abort_unref (SoupSession *session)
+{
+	g_object_add_weak_pointer (G_OBJECT (session), (gpointer *)&session);
+
+	soup_session_abort (session);
+	g_object_unref (session);
+
+	if (session) {
+		errors++;
+		debug_printf (1, "leaked SoupSession!\n");
+		g_object_remove_weak_pointer (G_OBJECT (session), (gpointer *)&session);
+	}
+}
+
 static gpointer run_server_thread (gpointer user_data);
 
 SoupServer *
@@ -271,6 +286,9 @@
 static void
 test_server_shutdown (void)
 {
+	g_object_add_weak_pointer (G_OBJECT (test_server),
+				   (gpointer *)&test_server);
+
 	if (server_thread) {
 		soup_add_completion (soup_server_get_async_context (test_server),
 				     idle_quit_server, test_server);
@@ -278,6 +296,11 @@
 	} else
 		soup_server_quit (test_server);
 	g_object_unref (test_server);
-}
-
 
+	if (test_server) {
+		errors++;
+		debug_printf (1, "leaked SoupServer!\n");
+		g_object_remove_weak_pointer (G_OBJECT (test_server),
+					      (gpointer *)&test_server);
+	}
+}

Modified: trunk/tests/test-utils.h
==============================================================================
--- trunk/tests/test-utils.h	(original)
+++ trunk/tests/test-utils.h	Tue Sep 30 15:43:17 2008
@@ -15,5 +15,7 @@
 void apache_cleanup (void);
 #endif
 
-SoupSession *soup_test_session_new (GType type, ...);
+SoupSession *soup_test_session_new         (GType type, ...);
+void         soup_test_session_abort_unref (SoupSession *session);
+
 SoupServer  *soup_test_server_new  (gboolean in_own_thread);

Modified: trunk/tests/xmlrpc-test.c
==============================================================================
--- trunk/tests/xmlrpc-test.c	(original)
+++ trunk/tests/xmlrpc-test.c	Tue Sep 30 15:43:17 2008
@@ -468,8 +468,7 @@
 	if (!test_fault_args ())
 		errors++;
 
-	soup_session_abort (session);
-	g_object_unref (session);
+	soup_test_session_abort_unref (session);
 
 	test_cleanup ();
 	return errors != 0;
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]