From 45524f2f5e9dc7eca5c439e4a8bff938abc17ee8 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 20:17:39 +0000 Subject: i should have given up x2 --- tests/http/test_fedclient.py | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) (limited to 'tests/http') diff --git a/tests/http/test_fedclient.py b/tests/http/test_fedclient.py index b03b37affe..0d0161d13d 100644 --- a/tests/http/test_fedclient.py +++ b/tests/http/test_fedclient.py @@ -268,6 +268,45 @@ class FederationClientTests(HomeserverTestCase): self.assertIsInstance(f.value, TimeoutError) + def test_client_requires_trailing_slashes(self): + """ + If a connection is made to a client but the client rejects it due to + requiring a trailing slash. We need to retry the request with a + trailing slash. Workaround for Synapse <=v0.99.2, explained in #3622. + """ + d = self.cl.get_json( + "testserv:8008", "foo/bar", try_trailing_slash_on_400=True, + ) + + 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") + + # Send the HTTP response + client.dataReceived( + b"HTTP/1.1 400 Bad Request\r\n" + b"Content-Type: application/json\r\n" + b"Content-Length: 59\r\n" + b"\r\n" + b'{"errcode":"M_UNRECOGNIZED","error":"Unrecognized request"}' + ) + + # We should get a successful response + r = self.successResultOf(d) + self.assertEqual(r.code, 400) + self.assertEqual(r, {}) + def test_client_sends_body(self): self.cl.post_json( "testserv:8008", "foo/bar", timeout=10000, -- cgit 1.4.1 From 86c60bda15d806cf4f6700dee2878de762cd240c Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 20:19:07 +0000 Subject: i should have given up x3 --- tests/http/test_fedclient.py | 39 --------------------------------------- 1 file changed, 39 deletions(-) (limited to 'tests/http') diff --git a/tests/http/test_fedclient.py b/tests/http/test_fedclient.py index 0d0161d13d..b03b37affe 100644 --- a/tests/http/test_fedclient.py +++ b/tests/http/test_fedclient.py @@ -268,45 +268,6 @@ class FederationClientTests(HomeserverTestCase): self.assertIsInstance(f.value, TimeoutError) - def test_client_requires_trailing_slashes(self): - """ - If a connection is made to a client but the client rejects it due to - requiring a trailing slash. We need to retry the request with a - trailing slash. Workaround for Synapse <=v0.99.2, explained in #3622. - """ - d = self.cl.get_json( - "testserv:8008", "foo/bar", try_trailing_slash_on_400=True, - ) - - 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") - - # Send the HTTP response - client.dataReceived( - b"HTTP/1.1 400 Bad Request\r\n" - b"Content-Type: application/json\r\n" - b"Content-Length: 59\r\n" - b"\r\n" - b'{"errcode":"M_UNRECOGNIZED","error":"Unrecognized request"}' - ) - - # We should get a successful response - r = self.successResultOf(d) - self.assertEqual(r.code, 400) - self.assertEqual(r, {}) - def test_client_sends_body(self): self.cl.post_json( "testserv:8008", "foo/bar", timeout=10000, -- cgit 1.4.1 From ecea5af491cd69933afe1dbea665d85c84d1d94e Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 21:21:03 +0000 Subject: Correct var name --- synapse/http/matrixfederationclient.py | 4 +-- tests/http/test_fedclient.py | 54 ++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 2 deletions(-) (limited to 'tests/http') diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 6b41639741..b27c4c1c38 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -238,10 +238,10 @@ class MatrixFederationHttpClient(object): # Enable backoff if initially disabled send_request_args["backoff_on_404"] = backoff_on_404 - send_request_args["path"] += "/" + # Add trailing slash + send_request_args["request"].path += "/" response = yield self._send_request(**send_request_args) - body = yield _handle_json_response( self.hs.get_reactor(), self.default_timeout, request, response, ) diff --git a/tests/http/test_fedclient.py b/tests/http/test_fedclient.py index b03b37affe..b45eee3a82 100644 --- a/tests/http/test_fedclient.py +++ b/tests/http/test_fedclient.py @@ -268,6 +268,60 @@ class FederationClientTests(HomeserverTestCase): self.assertIsInstance(f.value, TimeoutError) + def test_client_requires_trailing_slashes(self): + """ + If a connection is made to a client but the client rejects it due to + requiring a trailing slash. We need to retry the request with a + trailing slash. Workaround for Synapse <=v0.99.2, explained in #3622. + """ + d = self.cl.get_json( + "testserv:8008", "foo/bar", try_trailing_slash_on_400=True, + ) + + 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") + + # Send the HTTP response + client.dataReceived( + b"HTTP/1.1 400 Bad Request\r\n" + b"Content-Type: application/json\r\n" + b"Content-Length: 59\r\n" + b"\r\n" + b'{"errcode":"M_UNRECOGNIZED","error":"Unrecognized request"}' + ) + + # We should get a 400 response, then try again + self.pump() + + # We should get another request wiht a trailing slash + self.assertRegex(conn.value(), b"^GET /foo/bar/") + + # Send a happy response this time + client.dataReceived( + b"HTTP/1.1 200 OK\r\n" + b"Content-Type: application/json\r\n" + b"Content-Length: 2\r\n" + b"\r\n" + b'{}' + ) + + # We should get a successful response + r = self.successResultOf(d) + self.assertEqual(r.code, 200) + self.assertEqual(r, {}) + def test_client_sends_body(self): self.cl.post_json( "testserv:8008", "foo/bar", timeout=10000, -- cgit 1.4.1 From 621e7f37f1a7f32ff5046060f17a1da825f9ff8b Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 18 Mar 2019 17:45:54 +0000 Subject: Better exception handling --- synapse/http/matrixfederationclient.py | 33 +++++++++++++++++---------------- tests/http/test_fedclient.py | 5 +---- 2 files changed, 18 insertions(+), 20 deletions(-) (limited to 'tests/http') diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index b27c4c1c38..3c27686a89 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -221,30 +221,31 @@ class MatrixFederationHttpClient(object): """ 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, + ) except HttpResponseException as e: - # Received a 400. Raise unless we're retrying if not try_trailing_slash_on_400: + # Received an error >= 300. Raise unless we're retrying raise e - - # 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, - ) + except: + raise e # 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.2. - if not (response.code == 400 and body.get("errcode") == "M_UNRECOGNIZED"): - # Enable backoff if initially disabled - send_request_args["backoff_on_404"] = backoff_on_404 + # trailing slash on Synapse <= v0.99.2. + # Enable backoff if initially disabled + send_request_args["backoff_on_404"] = backoff_on_404 - # Add trailing slash - send_request_args["request"].path += "/" + # Add trailing slash + send_request_args["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(**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 b45eee3a82..84216db44f 100644 --- a/tests/http/test_fedclient.py +++ b/tests/http/test_fedclient.py @@ -272,7 +272,7 @@ class FederationClientTests(HomeserverTestCase): """ If a connection is made to a client but the client rejects it due to requiring a trailing slash. We need to retry the request with a - trailing slash. Workaround for Synapse <=v0.99.2, explained in #3622. + trailing slash. Workaround for Synapse <= v0.99.2, explained in #3622. """ d = self.cl.get_json( "testserv:8008", "foo/bar", try_trailing_slash_on_400=True, @@ -302,9 +302,6 @@ class FederationClientTests(HomeserverTestCase): b'{"errcode":"M_UNRECOGNIZED","error":"Unrecognized request"}' ) - # We should get a 400 response, then try again - self.pump() - # We should get another request wiht a trailing slash self.assertRegex(conn.value(), b"^GET /foo/bar/") -- cgit 1.4.1 From 94cb7939e4d608500396b65d5c68dd936503ba69 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 20 Mar 2019 10:50:44 +0000 Subject: Federation test fixed! --- tests/http/test_fedclient.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) (limited to 'tests/http') diff --git a/tests/http/test_fedclient.py b/tests/http/test_fedclient.py index 84216db44f..b1b3a025ef 100644 --- a/tests/http/test_fedclient.py +++ b/tests/http/test_fedclient.py @@ -278,6 +278,7 @@ class FederationClientTests(HomeserverTestCase): "testserv:8008", "foo/bar", try_trailing_slash_on_400=True, ) + # Send the request self.pump() # there should have been a call to connectTCP @@ -293,6 +294,9 @@ class FederationClientTests(HomeserverTestCase): # 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 400 Bad Request\r\n" @@ -302,7 +306,7 @@ class FederationClientTests(HomeserverTestCase): b'{"errcode":"M_UNRECOGNIZED","error":"Unrecognized request"}' ) - # We should get another request wiht a trailing slash + # We should get another request with a trailing slash self.assertRegex(conn.value(), b"^GET /foo/bar/") # Send a happy response this time @@ -316,7 +320,6 @@ class FederationClientTests(HomeserverTestCase): # We should get a successful response r = self.successResultOf(d) - self.assertEqual(r.code, 200) self.assertEqual(r, {}) def test_client_sends_body(self): -- cgit 1.4.1 From c69df5d5d37652b98d670a25c8c4291002af9242 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 20 Mar 2019 11:27:18 +0000 Subject: Fix comments. v0.99.2 -> v0.99.3 --- synapse/http/matrixfederationclient.py | 10 +++++----- tests/http/test_fedclient.py | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) (limited to 'tests/http') diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 1307508e5a..8312e4fc3f 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -198,7 +198,7 @@ class MatrixFederationHttpClient(object): ): """Wrapper for _send_request which can optionally retry the request upon receiving a combination of a 400 HTTP response code and a - 'M_UNRECOGNIZED' errcode. This is a workaround for Synapse <=v0.99.2 + 'M_UNRECOGNIZED' errcode. This is a workaround for Synapse <= v0.99.3 due to #3622. Args: @@ -237,7 +237,7 @@ class MatrixFederationHttpClient(object): # 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.2. + # trailing slash on Synapse <= v0.99.3. # Enable backoff if initially disabled send_request_args["backoff_on_404"] = backoff_on_404 @@ -560,7 +560,7 @@ class MatrixFederationHttpClient(object): requests). try_trailing_slash_on_400 (bool): True if on a 400 M_UNRECOGNIZED response we should try appending a trailing slash to the end - of the request. Workaround for #3622 in Synapse <=v0.99.2. This + of the request. Workaround for #3622 in Synapse <= v0.99.3. This will be attempted before backing off if backing off has been enabled. @@ -593,7 +593,7 @@ class MatrixFederationHttpClient(object): "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 + # 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, } @@ -681,7 +681,7 @@ class MatrixFederationHttpClient(object): and try the request anyway. try_trailing_slash_on_400 (bool): True if on a 400 M_UNRECOGNIZED response we should try appending a trailing slash to the end of - the request. Workaround for #3622 in Synapse <=v0.99.2. + the request. Workaround for #3622 in Synapse <= v0.99.3. Returns: Deferred[dict|list]: Succeeds when we get a 2xx HTTP response. The result will be the decoded JSON body. diff --git a/tests/http/test_fedclient.py b/tests/http/test_fedclient.py index b1b3a025ef..de5da1694a 100644 --- a/tests/http/test_fedclient.py +++ b/tests/http/test_fedclient.py @@ -272,7 +272,7 @@ class FederationClientTests(HomeserverTestCase): """ If a connection is made to a client but the client rejects it due to requiring a trailing slash. We need to retry the request with a - trailing slash. Workaround for Synapse <= v0.99.2, explained in #3622. + trailing slash. Workaround for Synapse <= v0.99.3, explained in #3622. """ d = self.cl.get_json( "testserv:8008", "foo/bar", try_trailing_slash_on_400=True, -- cgit 1.4.1 From cd36a1283b6036505b77f51a0cc860e8e5a0878d Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 20 Mar 2019 14:00:39 +0000 Subject: New test, fix issues --- synapse/http/matrixfederationclient.py | 77 +++++++++++++--------------------- tests/http/test_fedclient.py | 45 ++++++++++++++++++++ 2 files changed, 73 insertions(+), 49 deletions(-) (limited to 'tests/http') 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, -- cgit 1.4.1 From bb52a2e65308f10285d5d6290786e889d203f3db Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 20 Mar 2019 14:08:57 +0000 Subject: lint --- synapse/http/matrixfederationclient.py | 8 ++++++-- tests/http/test_fedclient.py | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) (limited to 'tests/http') diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index dd358536f1..2c1bcf66fd 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -219,7 +219,9 @@ class MatrixFederationHttpClient(object): Deferred[Dict]: Parsed JSON response body. """ try: - response = yield self._send_request(request, backoff_on_404=backoff_on_404, **send_request_args) + 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 @@ -234,7 +236,9 @@ class MatrixFederationHttpClient(object): # trailing slash on Synapse <= v0.99.3. request.path += "/" - response = yield self._send_request(request, backoff_on_404=backoff_on_404, **send_request_args) + response = yield self._send_request( + request, backoff_on_404=backoff_on_404, **send_request_args, + ) defer.returnValue(response) diff --git a/tests/http/test_fedclient.py b/tests/http/test_fedclient.py index fbe87d4d0b..cd8e086f86 100644 --- a/tests/http/test_fedclient.py +++ b/tests/http/test_fedclient.py @@ -365,7 +365,7 @@ class FederationClientTests(HomeserverTestCase): self.assertEqual(conn.value(), b"") # We should get a 404 failure response - r = self.failureResultOf(d) + self.failureResultOf(d) def test_client_sends_body(self): self.cl.post_json( -- cgit 1.4.1