[evolution-patches] 60693, soup thread safety bug

[Dan Winship <danw novell com>]

Siva sees this regularly in the groupwise backend. I'm not sure why he
does and everyone else seems not to, but the bug is definitely there.

The patch is largish because I updated a bunch of comments. There are
really only three small changes though:

     1. add soup_connection_release()
     2. call soup_connection_reserve() after a new connection attempt
        succeeds before adding the connection to the pool
     3. call soup_connection_release() on new connections in
        SoupSessionAsync, since we don't want that behavior there


Index: ChangeLog
===================================================================
RCS file: /cvs/gnome/libsoup/ChangeLog,v
retrieving revision 1.435
diff -u -r1.435 ChangeLog
--- ChangeLog	12 Jul 2004 19:08:26 -0000	1.435
+++ ChangeLog	13 Jul 2004 18:51:00 -0000
@@ -1,3 +1,21 @@
+2004-07-13  Dan Winship  <danw novell com>
+
+	* libsoup/soup-session.c (connect_result): If the connection
+	attempt succeeded, reserve the connection before releasing
+	host_lock. Otherwise, another thread might find it in the
+	connection pool before the caller can queue a message on it.
+	#60693
+
+	* libsoup/soup-session-async.c (got_connection): Call
+	soup_connection_release(), since we don't have a specific message
+	in mind for the connection, so we need it to be considered idle.
+
+	* libsoup/soup-connection.c (soup_connection_release): New
+	function, to undo a soup_connection_reserve().
+	(soup_connection_send_request, soup_connection_reserve,
+	soup_connection_authenticate, soup_connection_reauthenticate):
+	Document these
+
 2004-07-12  Dan Winship  <danw novell com>
 
 	* libsoup/soup-session-sync.c (send_message): signal the
Index: libsoup/soup-connection.c
===================================================================
RCS file: /cvs/gnome/libsoup/libsoup/soup-connection.c,v
retrieving revision 1.24
diff -u -r1.24 soup-connection.c
--- libsoup/soup-connection.c	15 Apr 2004 17:33:23 -0000	1.24
+++ libsoup/soup-connection.c	13 Jul 2004 18:51:00 -0000
@@ -673,6 +673,14 @@
 				   conn->priv->proxy_uri != NULL);
 }
 
+/**
+ * soup_connection_send_request:
+ * @conn: a #SoupConnection
+ * @req: a #SoupMessage
+ *
+ * Sends @req on @conn. This is a low-level function, intended for use
+ * by #SoupSession.
+ **/
 void
 soup_connection_send_request (SoupConnection *conn, SoupMessage *req)
 {
@@ -683,6 +691,15 @@
 	SOUP_CONNECTION_GET_CLASS (conn)->send_request (conn, req);
 }
 
+/**
+ * soup_connection_reserve:
+ * @conn: a #SoupConnection
+ *
+ * Marks @conn as "in use" despite not actually having a message on
+ * it. This is used by #SoupSession to keep it from accidentally
+ * trying to queue two messages on the same connection from different
+ * threads at the same time.
+ **/
 void
 soup_connection_reserve (SoupConnection *conn)
 {
@@ -691,6 +708,36 @@
 	conn->priv->in_use = TRUE;
 }
 
+/**
+ * soup_connection_release:
+ * @conn: a #SoupConnection
+ *
+ * Marks @conn as not "in use". This can be used to cancel the effect
+ * of a soup_session_reserve(). It is not necessary to call this
+ * after soup_connection_send_request().
+ **/
+void
+soup_connection_release (SoupConnection *conn)
+{
+	g_return_if_fail (SOUP_IS_CONNECTION (conn));
+
+	conn->priv->in_use = FALSE;
+}
+
+/**
+ * soup_connection_authenticate:
+ * @conn: a #SoupConnection
+ * @msg: the message to authenticate
+ * @auth_type: type of authentication to use
+ * @auth_realm: authentication realm
+ * @username: on successful return, will contain the username to
+ * authenticate with
+ * @password: on successful return, will contain the password to
+ * authenticate with
+ *
+ * Emits the %authenticate signal on @conn. For use by #SoupConnection
+ * subclasses.
+ **/
 void
 soup_connection_authenticate (SoupConnection *conn, SoupMessage *msg,
 			      const char *auth_type, const char *auth_realm,
@@ -700,6 +747,20 @@
 		       msg, auth_type, auth_realm, username, password);
 }
 
+/**
+ * soup_connection_authenticate:
+ * @conn: a #SoupConnection
+ * @msg: the message to authenticate
+ * @auth_type: type of authentication to use
+ * @auth_realm: authentication realm
+ * @username: on successful return, will contain the username to
+ * authenticate with
+ * @password: on successful return, will contain the password to
+ * authenticate with
+ *
+ * Emits the %reauthenticate signal on @conn. For use by
+ * #SoupConnection subclasses.
+ **/
 void
 soup_connection_reauthenticate (SoupConnection *conn, SoupMessage *msg,
 				const char *auth_type, const char *auth_realm,
Index: libsoup/soup-connection.h
===================================================================
RCS file: /cvs/gnome/libsoup/libsoup/soup-connection.h,v
retrieving revision 1.15
diff -u -r1.15 soup-connection.h
--- libsoup/soup-connection.h	19 Dec 2003 20:54:38 -0000	1.15
+++ libsoup/soup-connection.h	13 Jul 2004 18:51:00 -0000
@@ -70,7 +70,9 @@
 
 void            soup_connection_send_request   (SoupConnection   *conn,
 						SoupMessage      *req);
+
 void            soup_connection_reserve        (SoupConnection   *conn);
+void            soup_connection_release        (SoupConnection   *conn);
 
 /* protected */
 void            soup_connection_authenticate   (SoupConnection   *conn,
Index: libsoup/soup-session-async.c
===================================================================
RCS file: /cvs/gnome/libsoup/libsoup/soup-session-async.c,v
retrieving revision 1.3
diff -u -r1.3 soup-session-async.c
--- libsoup/soup-session-async.c	19 Dec 2003 20:54:38 -0000	1.3
+++ libsoup/soup-session-async.c	13 Jul 2004 18:51:00 -0000
@@ -94,16 +94,33 @@
 {
 	SoupSessionAsync *sa = user_data;
 
-	if (status == SOUP_STATUS_OK) {
-		g_signal_connect (conn, "disconnected",
-				  G_CALLBACK (connection_closed),
-				  sa);
+	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.
+		 */
+		run_queue (sa, FALSE);
+		return;
 	}
 
-	/* Either we just got a connection, or we just failed to
-	 * open a connection and so decremented the open connection
-	 * count by one. Either way, we need to run the queue now.
+	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.
 	 */
+	soup_connection_release (conn);
 	run_queue (sa, FALSE);
 }
 
Index: libsoup/soup-session.c
===================================================================
RCS file: /cvs/gnome/libsoup/libsoup/soup-session.c,v
retrieving revision 1.21
diff -u -r1.21 soup-session.c
--- libsoup/soup-session.c	6 Feb 2004 14:35:31 -0000	1.21
+++ libsoup/soup-session.c	13 Jul 2004 18:51:00 -0000
@@ -932,6 +932,7 @@
 	}
 
 	if (status == SOUP_STATUS_OK) {
+		soup_connection_reserve (conn);
 		host->connections = g_slist_prepend (host->connections, conn);
 		g_mutex_unlock (session->priv->host_lock);
 		return;
@@ -980,20 +981,32 @@
  * @is_new: on return, %TRUE if the returned connection is new and not
  * yet connected
  * 
- * Tries to find or create a connection for @msg. If there is an idle
- * connection to the relevant host available, then it will be returned
- * (with * is_new set to %FALSE). Otherwise, if it is possible to
- * create a new connection, one will be created and returned, with
- * * is_new set to %TRUE.
+ * Tries to find or create a connection for @msg.
  *
- * If no connection can be made, it will return %NULL. If @session has
+ * If there is an idle connection to the relevant host available, then
+ * that connection will be returned (with * is_new set to %FALSE). The
+ * connection will be marked "reserved", so the caller must call
+ * soup_connection_release() if it ends up not using the connection
+ * right away.
+ *
+ * If there is no idle connection available, but it is possible to
+ * create a new connection, then one will be created and returned,
+ * with * is_new set to %TRUE. The caller MUST then call
+ * soup_connection_connect_sync() or soup_connection_connect_async()
+ * to connect it. If the connection attempt succeeds, the connection
+ * will be marked "reserved" and added to @session's connection pool
+ * once it connects. If the connection attempt fails, the connection
+ * will be unreffed.
+ *
+ * If no connection is available and a new connection cannot be made,
+ * soup_session_get_connection() will return %NULL. If @session has
  * the maximum number of open connections open, but does not have the
- * maximum number of per-host connections open to the relevant host, then
- * * try_pruning will be set to %TRUE. In this case, the caller can
- * call soup_session_try_prune_connection() to close an idle connection,
- * and then try soup_session_get_connection() again. (If calling
- * soup_session_try_prune_connection() wouldn't help, then * try_pruning
- * is left untouched; it is NOT set to %FALSE.)
+ * maximum number of per-host connections open to the relevant host,
+ * then * try_pruning will be set to %TRUE. In this case, the caller
+ * can call soup_session_try_prune_connection() to close an idle
+ * connection, and then try soup_session_get_connection() again. (If
+ * calling soup_session_try_prune_connection() wouldn't help, then
+ * * try_pruning is left untouched; it is NOT set to %FALSE.)
  *
  * Return value: a #SoupConnection, or %NULL
  **/