[libsoup] Fix the retry-on-broken-connection codepath for SoupRequest

[Dan Winship <danw src gnome org>]

commit 3de849ac548c85be59c7708d3520e57b55ee1e10
Author: Dan Winship <danw gnome org>
Date:   Mon Jul 9 14:36:09 2012 -0400

    Fix the retry-on-broken-connection codepath for SoupRequest
    
    The retry-if-the-server-closes-the-connection-immediately code was
    only getting run in the SoupMessage API codepaths. Fit it into the
    right place in the SoupRequest paths too, and test that from
    connection-test.
    
    Might fix https://bugzilla.gnome.org/show_bug.cgi?id=679527 ?

 libsoup/soup-message-io.c    |   74 +++++++++++++------------
 libsoup/soup-misc-private.h  |   15 +++++
 libsoup/soup-session-async.c |    7 +++
 libsoup/soup-session-sync.c  |   11 +++-
 libsoup/soup-session.c       |   17 +------
 tests/connection-test.c      |  122 ++++++++++++++++++++++++++++++++++++++++-
 6 files changed, 190 insertions(+), 56 deletions(-)
---
diff --git a/libsoup/soup-message-io.c b/libsoup/soup-message-io.c
index 42ddb7d..db29b11 100644
--- a/libsoup/soup-message-io.c
+++ b/libsoup/soup-message-io.c
@@ -169,40 +169,6 @@ soup_message_io_finished (SoupMessage *msg)
 }
 
 static gboolean
-request_is_idempotent (SoupMessage *msg)
-{
-	/* FIXME */
-	return (msg->method == SOUP_METHOD_GET);
-}
-
-static void
-io_error (SoupMessage *msg, GError *error)
-{
-	SoupMessagePrivate *priv = SOUP_MESSAGE_GET_PRIVATE (msg);
-	SoupMessageIOData *io = priv->io_data;
-
-	if (error && error->domain == G_TLS_ERROR) {
-		soup_message_set_status_full (msg,
-					      SOUP_STATUS_SSL_FAILED,
-					      error->message);
-	} else if (io->mode == SOUP_MESSAGE_IO_CLIENT &&
-		   io->read_state <= SOUP_MESSAGE_IO_STATE_HEADERS &&
-		   io->read_header_buf->len == 0 &&
-		   soup_connection_get_ever_used (io->item->conn) &&
-		   !g_error_matches (error, G_IO_ERROR, G_IO_ERROR_TIMED_OUT) &&
-		   request_is_idempotent (msg)) {
-		/* Connection got closed, but we can safely try again */
-		io->item->state = SOUP_MESSAGE_RESTARTING;
-	} else if (!SOUP_STATUS_IS_TRANSPORT_ERROR (msg->status_code))
-		soup_message_set_status (msg, SOUP_STATUS_IO_ERROR);
-
-	if (error)
-		g_error_free (error);
-
-	soup_message_io_finished (msg);
-}
-
-static gboolean
 read_headers (SoupMessage *msg, GCancellable *cancellable, GError **error)
 {
 	SoupMessagePrivate *priv = SOUP_MESSAGE_GET_PRIVATE (msg);
@@ -815,6 +781,25 @@ soup_message_io_get_source (SoupMessage *msg, GCancellable *cancellable,
 }
 
 static gboolean
+request_is_restartable (SoupMessage *msg, GError *error)
+{
+	SoupMessagePrivate *priv = SOUP_MESSAGE_GET_PRIVATE (msg);
+	SoupMessageIOData *io = priv->io_data;
+
+	if (!io)
+		return FALSE;
+
+	return (io->mode == SOUP_MESSAGE_IO_CLIENT &&
+		io->read_state <= SOUP_MESSAGE_IO_STATE_HEADERS &&
+		io->read_header_buf->len == 0 &&
+		soup_connection_get_ever_used (io->item->conn) &&
+		!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_TIMED_OUT) &&
+		!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_WOULD_BLOCK) &&
+		error->domain != G_TLS_ERROR &&
+		SOUP_METHOD_IS_IDEMPOTENT (msg->method));
+}
+
+static gboolean
 io_run_until (SoupMessage *msg,
 	      SoupMessageIOState read_state, SoupMessageIOState write_state,
 	      GCancellable *cancellable, GError **error)
@@ -847,6 +832,15 @@ io_run_until (SoupMessage *msg,
 	}
 
 	if (my_error) {
+		if (request_is_restartable (msg, my_error)) {
+			/* Connection got closed, but we can safely try again */
+			g_error_free (my_error);
+			g_set_error_literal (error, SOUP_HTTP_ERROR,
+					     SOUP_STATUS_TRY_AGAIN, "");
+			g_object_unref (msg);
+			return FALSE;
+		}
+
 		g_propagate_error (error, my_error);
 		g_object_unref (msg);
 		return FALSE;
@@ -903,7 +897,17 @@ io_run (SoupMessage *msg, gpointer user_data)
 		io->io_source = soup_message_io_get_source (msg, NULL, io_run, msg);
 		g_source_attach (io->io_source, io->async_context);
 	} else if (error && priv->io_data == io) {
-		io_error (msg, error);
+		if (g_error_matches (error, SOUP_HTTP_ERROR, SOUP_STATUS_TRY_AGAIN))
+			io->item->state = SOUP_MESSAGE_RESTARTING;
+		else if (error->domain == G_TLS_ERROR) {
+			soup_message_set_status_full (msg,
+						      SOUP_STATUS_SSL_FAILED,
+						      error->message);
+		} else if (!SOUP_STATUS_IS_TRANSPORT_ERROR (msg->status_code))
+			soup_message_set_status (msg, SOUP_STATUS_IO_ERROR);
+
+		g_error_free (error);
+		soup_message_io_finished (msg);
 	}
 
 	g_object_unref (msg);
diff --git a/libsoup/soup-misc-private.h b/libsoup/soup-misc-private.h
index 06978e8..8276b8a 100644
--- a/libsoup/soup-misc-private.h
+++ b/libsoup/soup-misc-private.h
@@ -26,4 +26,19 @@ GIOStream *soup_socket_get_iostream   (SoupSocket *sock);
 #define SOUP_SOCKET_USE_PROXY     "use-proxy"
 SoupURI *soup_socket_get_http_proxy_uri (SoupSocket *sock);
 
+/* At some point it might be possible to mark additional methods
+ * safe or idempotent...
+ */
+#define SOUP_METHOD_IS_SAFE(method) (method == SOUP_METHOD_GET || \
+				     method == SOUP_METHOD_HEAD || \
+				     method == SOUP_METHOD_OPTIONS || \
+				     method == SOUP_METHOD_PROPFIND)
+
+#define SOUP_METHOD_IS_IDEMPOTENT(method) (method == SOUP_METHOD_GET || \
+					   method == SOUP_METHOD_HEAD || \
+					   method == SOUP_METHOD_OPTIONS || \
+					   method == SOUP_METHOD_PROPFIND || \
+					   method == SOUP_METHOD_PUT || \
+					   method == SOUP_METHOD_DELETE)
+
 #endif /* SOUP_URI_PRIVATE_H */
diff --git a/libsoup/soup-session-async.c b/libsoup/soup-session-async.c
index c9eda19..ad006ee 100644
--- a/libsoup/soup-session-async.c
+++ b/libsoup/soup-session-async.c
@@ -660,6 +660,13 @@ try_run_until_read (SoupMessageQueueItem *item)
 		return;
 	}
 
+	if (g_error_matches (error, SOUP_HTTP_ERROR, SOUP_STATUS_TRY_AGAIN)) {
+		item->state = SOUP_MESSAGE_RESTARTING;
+		soup_message_io_finished (item->msg);
+		g_error_free (error);
+		return;
+	}
+
 	if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_WOULD_BLOCK)) {
 		send_request_return_result (item, NULL, error);
 		return;
diff --git a/libsoup/soup-session-sync.c b/libsoup/soup-session-sync.c
index 3ac290a..1284176 100644
--- a/libsoup/soup-session-sync.c
+++ b/libsoup/soup-session-sync.c
@@ -493,8 +493,15 @@ soup_session_send_request (SoupSession   *session,
 			break;
 
 		/* Send request, read headers */
-		if (!soup_message_io_run_until_read (msg, item->cancellable, &my_error))
-			break;
+		if (!soup_message_io_run_until_read (msg, item->cancellable, &my_error)) {
+			if (g_error_matches (my_error, SOUP_HTTP_ERROR, SOUP_STATUS_TRY_AGAIN)) {
+				item->state = SOUP_MESSAGE_RESTARTING;
+				soup_message_io_finished (item->msg);
+				g_clear_error (&my_error);
+				continue;
+			} else
+				break;
+		}
 
 		stream = soup_message_io_get_response_istream (msg, &my_error);
 		if (!stream)
diff --git a/libsoup/soup-session.c b/libsoup/soup-session.c
index 23f9e94..bcd0be9 100644
--- a/libsoup/soup-session.c
+++ b/libsoup/soup-session.c
@@ -17,6 +17,7 @@
 #include "soup-connection.h"
 #include "soup-marshal.h"
 #include "soup-message-private.h"
+#include "soup-misc-private.h"
 #include "soup-message-queue.h"
 #include "soup-proxy-resolver-static.h"
 #include "soup-session-private.h"
@@ -850,22 +851,6 @@ auth_manager_authenticate (SoupAuthManager *manager, SoupMessage *msg,
 		session, msg, auth, retrying);
 }
 
-/* At some point it might be possible to mark additional methods
- * safe or idempotent...
- */
-#define SOUP_METHOD_IS_SAFE(method) (method == SOUP_METHOD_GET || \
-				     method == SOUP_METHOD_HEAD || \
-				     method == SOUP_METHOD_OPTIONS || \
-				     method == SOUP_METHOD_PROPFIND)
-
-#define SOUP_METHOD_IS_IDEMPOTENT(method) (method == SOUP_METHOD_GET || \
-					   method == SOUP_METHOD_HEAD || \
-					   method == SOUP_METHOD_OPTIONS || \
-					   method == SOUP_METHOD_PROPFIND || \
-					   method == SOUP_METHOD_PUT || \
-					   method == SOUP_METHOD_DELETE)
-
-
 #define SOUP_SESSION_WOULD_REDIRECT_AS_GET(session, msg) \
 	((msg)->status_code == SOUP_STATUS_SEE_OTHER || \
 	 ((msg)->status_code == SOUP_STATUS_FOUND && \
diff --git a/tests/connection-test.c b/tests/connection-test.c
index ee2a78b..ddf0c98 100644
--- a/tests/connection-test.c
+++ b/tests/connection-test.c
@@ -238,7 +238,6 @@ request_started_socket_collector (SoupSession *session, SoupMessage *msg,
 
 	debug_printf (1, "      socket queue overflowed!\n");
 	errors++;
-	soup_session_cancel_message (session, msg, SOUP_STATUS_CANCELLED);
 }
 
 static void
@@ -298,21 +297,138 @@ do_timeout_test_for_session (SoupSession *session)
 }
 
 static void
+do_timeout_req_test_for_session (SoupSession *session)
+{
+	SoupRequester *requester;
+	SoupRequest *req;
+	SoupMessage *msg;
+	GInputStream *stream;
+	SoupSocket *sockets[4] = { NULL, NULL, NULL, NULL };
+	SoupURI *timeout_uri;
+	GError *error = NULL;
+	int i;
+
+	requester = soup_requester_new ();
+	soup_session_add_feature (session, SOUP_SESSION_FEATURE (requester));
+	g_object_unref (requester);
+
+	g_signal_connect (session, "request-started",
+			  G_CALLBACK (request_started_socket_collector),
+			  &sockets);
+
+	debug_printf (1, "    First request\n");
+	timeout_uri = soup_uri_new_with_base (base_uri, "/timeout-persistent");
+	req = soup_requester_request_uri (requester, timeout_uri, NULL);
+	soup_uri_free (timeout_uri);
+
+	if (SOUP_IS_SESSION_SYNC (session))
+		stream = soup_request_send (req, NULL, &error);
+	else
+		stream = soup_test_request_send_async_as_sync (req, NULL, &error);
+
+	if (!stream) {
+		debug_printf (1, "      Unexpected error on send: %s\n",
+			      error->message);
+		errors++;
+		g_clear_error (&error);
+	} else {
+		if (SOUP_IS_SESSION_SYNC (session))
+			g_input_stream_close (stream, NULL, &error);
+		else
+			soup_test_stream_close_async_as_sync (stream, NULL, &error);
+		if (error) {
+			debug_printf (1, "  Unexpected error on close: %s\n",
+				      error->message);
+			errors++;
+			g_clear_error (&error);
+		}
+	}
+
+	if (sockets[1]) {
+		debug_printf (1, "      Message was retried??\n");
+		errors++;
+		sockets[1] = sockets[2] = sockets[3] = NULL;
+	}
+	g_object_unref (req);
+
+	debug_printf (1, "    Second request\n");
+	req = soup_requester_request_uri (requester, base_uri, NULL);
+
+	if (SOUP_IS_SESSION_SYNC (session))
+		stream = soup_request_send (req, NULL, &error);
+	else
+		stream = soup_test_request_send_async_as_sync (req, NULL, &error);
+
+	if (!stream) {
+		debug_printf (1, "      Unexpected error on send: %s\n",
+			      error->message);
+		errors++;
+		g_clear_error (&error);
+	} else {
+		if (SOUP_IS_SESSION_SYNC (session))
+			g_input_stream_close (stream, NULL, &error);
+		else
+			soup_test_stream_close_async_as_sync (stream, NULL, &error);
+		if (error) {
+			debug_printf (1, "  Unexpected error on close: %s\n",
+				      error->message);
+			errors++;
+			g_clear_error (&error);
+		}
+	}
+
+	msg = soup_request_http_get_message (SOUP_REQUEST_HTTP (req));
+	if (msg->status_code != SOUP_STATUS_OK) {
+		debug_printf (1, "      Unexpected response: %d %s\n",
+			      msg->status_code, msg->reason_phrase);
+		errors++;
+	}
+	if (sockets[1] != sockets[0]) {
+		debug_printf (1, "      Message was not retried on existing connection\n");
+		errors++;
+	} else if (!sockets[2]) {
+		debug_printf (1, "      Message was not retried after disconnect\n");
+		errors++;
+	} else if (sockets[2] == sockets[1]) {
+		debug_printf (1, "      Message was retried on closed connection??\n");
+		errors++;
+	} else if (sockets[3]) {
+		debug_printf (1, "      Message was retried again??\n");
+		errors++;
+	}
+	g_object_unref (msg);
+	g_object_unref (req);
+
+	for (i = 0; sockets[i]; i++)
+		g_object_unref (sockets[i]);
+}
+
+static void
 do_persistent_connection_timeout_test (void)
 {
 	SoupSession *session;
 
 	debug_printf (1, "\nUnexpected timing out of persistent connections\n");
 
-	debug_printf (1, "  Async session\n");
+	debug_printf (1, "  Async session, message API\n");
 	session = soup_test_session_new (SOUP_TYPE_SESSION_ASYNC, NULL);
 	do_timeout_test_for_session (session);
 	soup_test_session_abort_unref (session);
 
-	debug_printf (1, "  Sync session\n");
+	session = soup_test_session_new (SOUP_TYPE_SESSION_ASYNC,
+					 SOUP_SESSION_USE_THREAD_CONTEXT, TRUE,
+					 NULL);
+	do_timeout_req_test_for_session (session);
+	soup_test_session_abort_unref (session);
+
+	debug_printf (1, "  Sync session, message API\n");
 	session = soup_test_session_new (SOUP_TYPE_SESSION_SYNC, NULL);
 	do_timeout_test_for_session (session);
 	soup_test_session_abort_unref (session);
+
+	session = soup_test_session_new (SOUP_TYPE_SESSION_SYNC, NULL);
+	do_timeout_req_test_for_session (session);
+	soup_test_session_abort_unref (session);
 }
 
 static GMainLoop *max_conns_loop;