Re: soup patch, 29731

Dan Winship <[email protected]> 28 Oct 2002 10:20:12 -0500
Newsgroups gmane.comp.gnome.ximian.soup
Message-ID <[email protected]> 
On Mon, 2002-10-28 at 01:09, Not Zed wrote: 
> Patch addresses 29731, I'm not sure all cases are completely handled,
> for example a successful immediate connect, should that return NULL or
> not?

It should return NULL, since you can't cancel the attempt at that point.

>   But otherwise it fixes the crash in 29371, tho maybe it could use
> some neatening (e.g. unused enum's).

Alternate patch attached; it just uses state->tcp_id and
state->inetaddr_id to distinguish the immediate callback case from the
async case, which makes it clearer I think.

> There is also a small patch to fix another problem with cancelled
> connections never being discounted, which means after 10 cancellations,
> connections are never re-initiated.  Which basically stops image loading
> working at all.

That part looks good.

> And finally a patch (request of Larry) to turn of the PTRACE debugging,
> as it just hinders doing any debugging with it turned on.

Hm... Alex added the ptrace code because otherwise setting breakpoints
on certain functions (like soup_gethostbyname or g_strdup) would make
hostname lookups fail reliably. It must not work right with multiple
threads or something.

-- Dan
29731.diff (text/x-patch, 3.5 KB) 
Index: soup-socket.c
===================================================================
RCS file: /cvs/gnome/soup/src/libsoup/soup-socket.c,v
retrieving revision 1.24.2.9
diff -u -r1.24.2.9 soup-socket.c
--- soup-socket.c	7 Mar 2002 17:14:33 -0000	1.24.2.9
+++ soup-socket.c	28 Oct 2002 14:43:05 -0000
@@ -315,8 +315,6 @@
 	SoupSocketConnectFn func = state->func;
 	gpointer user_data = state->data;
 
-	g_free (state);
-
 	if (status == SOUP_SOCKET_NEW_STATUS_OK)
 		(*func) (socket,
 			 SOUP_SOCKET_CONNECT_ERROR_NONE,
@@ -325,6 +323,9 @@
 		(*func) (NULL,
 			 SOUP_SOCKET_CONNECT_ERROR_NETWORK,
 			 user_data);
+
+	if (state->tcp_id)
+		g_free (state);
 }
 
 static void
@@ -335,31 +336,23 @@
 	SoupSocketConnectState* state = (SoupSocketConnectState*) data;
 
 	if (status == SOUP_ADDRESS_STATUS_OK) {
-		gpointer tcp_id;
-
-		state->inetaddr_id = NULL;
-
-		tcp_id = soup_socket_new (inetaddr,
-					  soup_socket_connect_tcp_cb,
-					  state);
-		/* 
-		 * NOTE: soup_socket_new can fail immediately and call our
-		 * callback which will delete the state.  
-		 */
-		if (tcp_id)
-			state->tcp_id = tcp_id;
-
+		state->tcp_id = soup_socket_new (inetaddr,
+						 soup_socket_connect_tcp_cb,
+						 state);
 		soup_address_unref (inetaddr);
 	} else {
 		SoupSocketConnectFn func = state->func;
 		gpointer user_data = state->data;
 
-		g_free (state);
-
 		(*func) (NULL, 
 			 SOUP_SOCKET_CONNECT_ERROR_ADDR_RESOLVE, 
 			 user_data);
 	}
+
+	if (state->inetaddr_id && !state->tcp_id)
+		g_free (state);
+	else
+		state->inetaddr_id = NULL;
 }
 
 /**
@@ -377,8 +370,8 @@
  * returns.  It will call the callback if there is a failure.
  *
  * Returns: ID of the connection which can be used with
- * soup_socket_connect_cancel() to cancel it; NULL on
- * failure.
+ * soup_socket_connect_cancel() to cancel it; NULL if it succeeds
+ * or fails immediately.
  **/
 SoupSocketConnectId
 soup_socket_connect (const gchar*        hostname,
@@ -388,7 +381,6 @@
 {
 	SoupSocketConnectState* state;
 	SoupAddress *cached_addr;
-	gpointer addr_id, tcp_id;
 
 	g_return_val_if_fail (hostname != NULL, NULL);
 	g_return_val_if_fail (func != NULL, NULL);
@@ -400,35 +392,26 @@
 	/* Check if a cached version of the address already exists */
 	cached_addr = soup_address_lookup_in_cache (hostname, port);
 	if (cached_addr) {
-		tcp_id = soup_socket_new (cached_addr,
-					  soup_socket_connect_tcp_cb,
-					  state);
+		state->tcp_id = soup_socket_new (cached_addr,
+						 soup_socket_connect_tcp_cb,
+						 state);
 		soup_address_unref (cached_addr);
-
-		/* 
-		 * NOTE: soup_socket_new can fail immediately and call our
-		 * callback which will delete the state.  
-		 */
-		if (tcp_id) {
-			state->tcp_id = tcp_id;
-			return state;
-		} else
-			return NULL;
 	} else {
-		addr_id = soup_address_new (hostname,
-					    port,
-					    soup_socket_connect_inetaddr_cb,
-					    state);
-
-		/* 
-		 * NOTE: soup_address_new can fail immediately and call our
-		 * callback which will delete the state.  
+		state->inetaddr_id = soup_address_new (hostname,
+						       port,
+						       soup_socket_connect_inetaddr_cb,
+						       state);
+		/* NOTE: soup_address_new could succeed immediately
+		 * and call our callback, in which case state->inetaddr_id
+		 * will be NULL but state->tcp_id may be set.
 		 */
-		if (addr_id) {
-			state->inetaddr_id = addr_id;
-			return state;
-		} else
-			return NULL;
+	}
+
+	if (state->tcp_id || state->inetaddr_id)
+		return state;
+	else {
+		g_free (state);
+		return NULL;
 	}
 }