Re: [evolution-patches] 60693, soup thread safety bug

[Sivaiah N <snallagatla novell com>]

I applied this patch on my local libsoup sources and tried, works fine
so far

Thanks,
Siva
On Wed, 2004-07-14 at 00:30, Dan Winship wrote:
> 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
>   **/