New test, fix issues

2 files changed, 73 insertions, 49 deletions

diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py
index 8312e4fc3f..dd358536f1 100644
--- a/synapse/http/matrixfederationclient.py
+++ b/synapse/http/matrixfederationclient.py
@@ -194,7 +194,7 @@ class MatrixFederationHttpClient(object):
         request,
         try_trailing_slash_on_400=False,
         backoff_on_404=False,
-        send_request_args={},
+        **send_request_args
     ):
         """Wrapper for _send_request which can optionally retry the request
         upon receiving a combination of a 400 HTTP response code and a
@@ -206,9 +206,8 @@ class MatrixFederationHttpClient(object):
             try_trailing_slash_on_400 (bool): Whether on receiving a 400
                 'M_UNRECOGNIZED' from the server to retry the request with a
                 trailing slash appended to the request path.
-            404_backoff (bool): Whether to backoff on 404 when making a
-                request with a trailing slash (only affects request if
-                try_trailing_slash_on_400 is True).
+            backoff_on_404 (bool): Whether to backoff on 404 when making a
+                request with a trailing slash.
             send_request_args (Dict): A dictionary of arguments to pass to
                 `_send_request()`.
 
@@ -220,36 +219,24 @@ class MatrixFederationHttpClient(object):
             Deferred[Dict]: Parsed JSON response body.
         """
         try:
-            response = yield self._send_request(**send_request_args)
-
-            # Check if it's necessary to retry with a trailing slash
-            body = yield _handle_json_response(
-                self.hs.get_reactor(), self.default_timeout, request, response,
-            )
-
-            defer.returnValue(body)
+            response = yield self._send_request(request, backoff_on_404=backoff_on_404, **send_request_args)
         except HttpResponseException as e:
+            # Received an HTTP error > 300. Check if it meets the requirements
+            # to retry with a trailing slash
             if not try_trailing_slash_on_400:
-                # Received an error >= 300. Raise unless we're retrying
-                raise e
-        except Exception as e:
-            raise e
+                raise
 
-        # Retry with a trailing slash if we received a 400 with
-        # 'M_UNRECOGNIZED' which some endpoints can return when omitting a
-        # trailing slash on Synapse <= v0.99.3.
-        # Enable backoff if initially disabled
-        send_request_args["backoff_on_404"] = backoff_on_404
+            if e.code != 400 or e.to_synapse_error().errcode != "M_UNRECOGNIZED":
+                raise
 
-        # Add trailing slash
-        send_request_args["request"].path += "/"
+            # Retry with a trailing slash if we received a 400 with
+            # 'M_UNRECOGNIZED' which some endpoints can return when omitting a
+            # trailing slash on Synapse <= v0.99.3.
+            request.path += "/"
 
-        response = yield self._send_request(**send_request_args)
-        body = yield _handle_json_response(
-            self.hs.get_reactor(), self.default_timeout, request, response,
-        )
+            response = yield self._send_request(request, backoff_on_404=backoff_on_404, **send_request_args)
 
-        defer.returnValue(body)
+        defer.returnValue(response)
 
     @defer.inlineCallbacks
     def _send_request(
@@ -587,19 +574,13 @@ class MatrixFederationHttpClient(object):
             json=data,
         )
 
-        send_request_args = {
-            "request": request,
-            "long_retries": long_retries,
-            "timeout": timeout,
-            "ignore_backoff": ignore_backoff,
-            # Do not backoff on the initial request if we're trying again with
-            # trailing slashes. Otherwise we may end up waiting to contact a
-            # server that is actually up
-            "backoff_on_404": False if try_trailing_slash_on_400 else backoff_on_404,
-        }
+        response = yield self._send_request_with_optional_trailing_slash(
+            request, try_trailing_slash_on_400, backoff_on_404,
+            long_retries=long_retries, timeout=timeout, ignore_backoff=ignore_backoff,
+        )
 
-        body = yield self._send_request_with_optional_trailing_slash(
-            request, try_trailing_slash_on_400, backoff_on_404, send_request_args,
+        body = yield _handle_json_response(
+            self.hs.get_reactor(), self.default_timeout, request, response,
         )
 
         defer.returnValue(body)
@@ -707,16 +688,14 @@ class MatrixFederationHttpClient(object):
             query=args,
         )
 
-        send_request_args = {
-            "request": request,
-            "retry_on_dns_fail": retry_on_dns_fail,
-            "timeout": timeout,
-            "ignore_backoff": ignore_backoff,
-            "backoff_on_404": False,
-        }
+        response = yield self._send_request_with_optional_trailing_slash(
+            request, try_trailing_slash_on_400, False,
+            retry_on_dns_fail=retry_on_dns_fail, timeout=timeout,
+            ignore_backoff=ignore_backoff,
+        )
 
-        body = yield self._send_request_with_optional_trailing_slash(
-            request, try_trailing_slash_on_400, False, send_request_args,
+        body = yield _handle_json_response(
+            self.hs.get_reactor(), self.default_timeout, request, response,
         )
 
         defer.returnValue(body)
diff --git a/tests/http/test_fedclient.py b/tests/http/test_fedclient.py
index de5da1694a..fbe87d4d0b 100644
--- a/tests/http/test_fedclient.py
+++ b/tests/http/test_fedclient.py
@@ -322,6 +322,51 @@ class FederationClientTests(HomeserverTestCase):
         r = self.successResultOf(d)
         self.assertEqual(r, {})
 
+    def test_client_does_not_retry_on_400_plus(self):
+        """
+        Another test for trailing slashes but now test that we don't retry on
+        trailing slashes on a non-400/M_UNRECOGNIZED response.
+
+        See test_client_requires_trailing_slashes() for context.
+        """
+        d = self.cl.get_json(
+            "testserv:8008", "foo/bar", try_trailing_slash_on_400=True,
+        )
+
+        # Send the request
+        self.pump()
+
+        # there should have been a call to connectTCP
+        clients = self.reactor.tcpClients
+        self.assertEqual(len(clients), 1)
+        (_host, _port, factory, _timeout, _bindAddress) = clients[0]
+
+        # complete the connection and wire it up to a fake transport
+        client = factory.buildProtocol(None)
+        conn = StringTransport()
+        client.makeConnection(conn)
+
+        # that should have made it send the request to the connection
+        self.assertRegex(conn.value(), b"^GET /foo/bar")
+
+        # Clear the original request data before sending a response
+        conn.clear()
+
+        # Send the HTTP response
+        client.dataReceived(
+            b"HTTP/1.1 404 Not Found\r\n"
+            b"Content-Type: application/json\r\n"
+            b"Content-Length: 2\r\n"
+            b"\r\n"
+            b"{}"
+        )
+
+        # We should not get another request
+        self.assertEqual(conn.value(), b"")
+
+        # We should get a 404 failure response
+        r = self.failureResultOf(d)
+
     def test_client_sends_body(self):
         self.cl.post_json(
             "testserv:8008", "foo/bar", timeout=10000,