From 525dd02bbe3700f4cd53db17d54183b03ac16c30 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 8 Mar 2019 16:55:52 +0000 Subject: [PATCH 01/40] Remove trailing slashes from outbound federation requests --- synapse/federation/transport/client.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py index 8e2be218e..492cd4e64 100644 --- a/synapse/federation/transport/client.py +++ b/synapse/federation/transport/client.py @@ -51,7 +51,7 @@ class TransportLayerClient(object): logger.debug("get_room_state dest=%s, room=%s", destination, room_id) - path = _create_v1_path("/state/%s/", room_id) + path = _create_v1_path("/state/%s", room_id) return self.client.get_json( destination, path=path, args={"event_id": event_id}, ) @@ -73,7 +73,7 @@ class TransportLayerClient(object): logger.debug("get_room_state_ids dest=%s, room=%s", destination, room_id) - path = _create_v1_path("/state_ids/%s/", room_id) + path = _create_v1_path("/state_ids/%s", room_id) return self.client.get_json( destination, path=path, args={"event_id": event_id}, ) @@ -95,7 +95,7 @@ class TransportLayerClient(object): logger.debug("get_pdu dest=%s, event_id=%s", destination, event_id) - path = _create_v1_path("/event/%s/", event_id) + path = _create_v1_path("/event/%s", event_id) return self.client.get_json(destination, path=path, timeout=timeout) @log_function @@ -121,7 +121,7 @@ class TransportLayerClient(object): # TODO: raise? return - path = _create_v1_path("/backfill/%s/", room_id) + path = _create_v1_path("/backfill/%s", room_id) args = { "v": event_tuples, @@ -167,7 +167,7 @@ class TransportLayerClient(object): # generated by the json_data_callback. json_data = transaction.get_dict() - path = _create_v1_path("/send/%s/", transaction.transaction_id) + path = _create_v1_path("/send/%s", transaction.transaction_id) response = yield self.client.put_json( transaction.destination, @@ -959,7 +959,7 @@ def _create_v1_path(path, *args): Example: - _create_v1_path("/event/%s/", event_id) + _create_v1_path("/event/%s", event_id) Args: path (str): String template for the path @@ -980,7 +980,7 @@ def _create_v2_path(path, *args): Example: - _create_v2_path("/event/%s/", event_id) + _create_v2_path("/event/%s", event_id) Args: path (str): String template for the path From 64ff11019e11a712278fdb4fbc0a5f8307f83ddb Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 8 Mar 2019 18:22:47 +0000 Subject: [PATCH 02/40] Retry certain federation requests on 404 --- synapse/federation/transport/client.py | 10 +++--- synapse/http/matrixfederationclient.py | 45 ++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 5 deletions(-) diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py index 492cd4e64..2bd0e0040 100644 --- a/synapse/federation/transport/client.py +++ b/synapse/federation/transport/client.py @@ -52,7 +52,7 @@ class TransportLayerClient(object): destination, room_id) path = _create_v1_path("/state/%s", room_id) - return self.client.get_json( + return self.client.get_json_with_trailing_slashes_on_404( destination, path=path, args={"event_id": event_id}, ) @@ -74,7 +74,7 @@ class TransportLayerClient(object): destination, room_id) path = _create_v1_path("/state_ids/%s", room_id) - return self.client.get_json( + return self.client.get_json_with_trailing_slashes_on_404( destination, path=path, args={"event_id": event_id}, ) @@ -96,7 +96,7 @@ class TransportLayerClient(object): destination, event_id) path = _create_v1_path("/event/%s", event_id) - return self.client.get_json(destination, path=path, timeout=timeout) + return self.client.get_json_with_trailing_slashes_on_404(destination, path=path, timeout=timeout) @log_function def backfill(self, destination, room_id, event_tuples, limit): @@ -128,7 +128,7 @@ class TransportLayerClient(object): "limit": [str(limit)], } - return self.client.get_json( + return self.client.get_json_with_trailing_slashes_on_404( destination, path=path, args=args, @@ -169,7 +169,7 @@ class TransportLayerClient(object): path = _create_v1_path("/send/%s", transaction.transaction_id) - response = yield self.client.put_json( + response = yield self.client.put_json_with_trailing_slashes_on_404( transaction.destination, path=path, data=json_data, diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 1682c9af1..8776639d6 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -643,6 +643,51 @@ class MatrixFederationHttpClient(object): ) defer.returnValue(body) + @defer.inlineCallbacks + def get_json_with_trailing_slashes_on_404(self, args={}): + """Runs client.get_json under the hood, but if receiving a 404, tries + the request again with a trailing slash. This is a result of removing + trailing slashes from some federation endpoints and in an effort to + remain backwards compatible with older versions of Synapse, we try + again if a server requires a trailing slash. + + Args: + args (dict): A dictionary of arguments matching those provided by put_json. + Returns: + Deferred[dict|list]: Succeeds when we get a 2xx HTTP response. The + result will be the decoded JSON body. + """ + response = yield self.get_json(**args) + + # Retry with a trailing slash if we received a 404 + if response.code == 404: + args["path"] += "/" + response = yield self.get_json(**args) + + defer.returnValue(response) + + @defer.inlineCallbacks + def put_json_with_trailing_slashes_on_404(self, args={}): + """Runs client.put_json under the hood, but if receiving a 404, tries + the request again with a trailing slash. + + See get_json_with_trailing_slashes_on_404 for more details. + + Args: + args (dict): A dictionary of arguments matching those provided by put_json. + Returns: + Deferred[dict|list]: Succeeds when we get a 2xx HTTP response. The + result will be the decoded JSON body. + """ + response = yield self.put_json(**args) + + # Retry with a trailing slash if we received a 404 + if response.code == 404: + args["path"] += "/" + response = yield self.put_json(**args) + + defer.returnValue(response) + @defer.inlineCallbacks def delete_json(self, destination, path, long_retries=False, timeout=None, ignore_backoff=False, args={}): From f8740d57de6781a966eee3f7e1cfa4d7257da7c9 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 8 Mar 2019 18:23:54 +0000 Subject: [PATCH 03/40] Add changelog --- changelog.d/4840.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/4840.feature diff --git a/changelog.d/4840.feature b/changelog.d/4840.feature new file mode 100644 index 000000000..9d1fd5905 --- /dev/null +++ b/changelog.d/4840.feature @@ -0,0 +1 @@ +Remove trailing slashes from certain outbound federation requests. Retry if receiving a 404. Context: #3622. \ No newline at end of file From a5dd335cd867f71fe3833fd8edc4cb284faa2415 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 8 Mar 2019 18:25:59 +0000 Subject: [PATCH 04/40] lint --- synapse/federation/transport/client.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py index 2bd0e0040..68808e920 100644 --- a/synapse/federation/transport/client.py +++ b/synapse/federation/transport/client.py @@ -96,7 +96,9 @@ class TransportLayerClient(object): destination, event_id) path = _create_v1_path("/event/%s", event_id) - return self.client.get_json_with_trailing_slashes_on_404(destination, path=path, timeout=timeout) + return self.client.get_json_with_trailing_slashes_on_404( + destination, path=path, timeout=timeout, + ) @log_function def backfill(self, destination, room_id, event_tuples, limit): From 66f205e93d933b18a2ba6fe7fa5c9dc33e4190f6 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 11 Mar 2019 18:01:58 +0000 Subject: [PATCH 05/40] We're calling different functions now --- tests/handlers/test_typing.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/handlers/test_typing.py b/tests/handlers/test_typing.py index b8e97390d..8589a2ee9 100644 --- a/tests/handlers/test_typing.py +++ b/tests/handlers/test_typing.py @@ -177,7 +177,7 @@ class TypingNotificationsTestCase(unittest.HomeserverTestCase): timeout=20000, )) - put_json = self.hs.get_http_client().put_json + put_json = self.hs.get_http_client().put_json_with_trailing_slashes_on_404( put_json.assert_called_once_with( "farm", path="/_matrix/federation/v1/send/1000000", @@ -254,7 +254,7 @@ class TypingNotificationsTestCase(unittest.HomeserverTestCase): [call('typing_key', 1, rooms=[ROOM_ID])] ) - put_json = self.hs.get_http_client().put_json + put_json = self.hs.get_http_client().put_json_with_trailing_slashes_on_404( put_json.assert_called_once_with( "farm", path="/_matrix/federation/v1/send/1000000", From 802cb5dcf05d99d7bd1873c2e25df1ab6bf9d4d2 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 11 Mar 2019 18:08:28 +0000 Subject: [PATCH 06/40] Fix syntax error --- tests/handlers/test_typing.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/handlers/test_typing.py b/tests/handlers/test_typing.py index 8589a2ee9..75d00c920 100644 --- a/tests/handlers/test_typing.py +++ b/tests/handlers/test_typing.py @@ -254,7 +254,7 @@ class TypingNotificationsTestCase(unittest.HomeserverTestCase): [call('typing_key', 1, rooms=[ROOM_ID])] ) - put_json = self.hs.get_http_client().put_json_with_trailing_slashes_on_404( + put_json = self.hs.get_http_client().put_json_with_trailing_slashes_on_404 put_json.assert_called_once_with( "farm", path="/_matrix/federation/v1/send/1000000", From 4868b120297b1f7951379821483ab8f6a6aa0903 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 11 Mar 2019 18:22:26 +0000 Subject: [PATCH 07/40] and again --- tests/handlers/test_typing.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/handlers/test_typing.py b/tests/handlers/test_typing.py index 75d00c920..5a3670c93 100644 --- a/tests/handlers/test_typing.py +++ b/tests/handlers/test_typing.py @@ -177,7 +177,7 @@ class TypingNotificationsTestCase(unittest.HomeserverTestCase): timeout=20000, )) - put_json = self.hs.get_http_client().put_json_with_trailing_slashes_on_404( + put_json = self.hs.get_http_client().put_json_with_trailing_slashes_on_404 put_json.assert_called_once_with( "farm", path="/_matrix/federation/v1/send/1000000", From 0ea8582f8bd83fc9f4cba80c0462f44583e059a3 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 12 Mar 2019 14:11:11 +0000 Subject: [PATCH 08/40] Cleaner way of implementing trailing slashes --- synapse/federation/transport/client.py | 15 ++-- synapse/http/matrixfederationclient.py | 115 +++++++++++-------------- tests/handlers/test_typing.py | 6 +- 3 files changed, 66 insertions(+), 70 deletions(-) diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py index 68808e920..37f37b3d4 100644 --- a/synapse/federation/transport/client.py +++ b/synapse/federation/transport/client.py @@ -52,8 +52,9 @@ class TransportLayerClient(object): destination, room_id) path = _create_v1_path("/state/%s", room_id) - return self.client.get_json_with_trailing_slashes_on_404( + return self.client.get_json( destination, path=path, args={"event_id": event_id}, + try_trailing_slash_on_404=True, ) @log_function @@ -74,8 +75,9 @@ class TransportLayerClient(object): destination, room_id) path = _create_v1_path("/state_ids/%s", room_id) - return self.client.get_json_with_trailing_slashes_on_404( + return self.client.get_json( destination, path=path, args={"event_id": event_id}, + try_trailing_slash_on_404=True, ) @log_function @@ -96,8 +98,9 @@ class TransportLayerClient(object): destination, event_id) path = _create_v1_path("/event/%s", event_id) - return self.client.get_json_with_trailing_slashes_on_404( + return self.client.get_json( destination, path=path, timeout=timeout, + try_trailing_slash_on_404=True, ) @log_function @@ -130,10 +133,11 @@ class TransportLayerClient(object): "limit": [str(limit)], } - return self.client.get_json_with_trailing_slashes_on_404( + return self.client.get_json( destination, path=path, args=args, + try_trailing_slash_on_404=True, ) @defer.inlineCallbacks @@ -171,13 +175,14 @@ class TransportLayerClient(object): path = _create_v1_path("/send/%s", transaction.transaction_id) - response = yield self.client.put_json_with_trailing_slashes_on_404( + response = yield self.client.put_json( transaction.destination, path=path, data=json_data, json_data_callback=json_data_callback, long_retries=True, backoff_on_404=True, # If we get a 404 the other side has gone + try_trailing_slash_on_404=True, ) defer.returnValue(response) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 8776639d6..fec7f5f88 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -196,7 +196,8 @@ class MatrixFederationHttpClient(object): timeout=None, long_retries=False, ignore_backoff=False, - backoff_on_404=False + backoff_on_404=False, + try_trailing_slash_on_404=False, ): """ Sends a request to the given server. @@ -212,6 +213,11 @@ class MatrixFederationHttpClient(object): backoff_on_404 (bool): Back off if we get a 404 + try_trailing_slash_on_404 (bool): True if on a 404 response we + should try appending a trailing slash to the end of the + request. This will be attempted before backing off if backing + off has been enabled. + Returns: Deferred[twisted.web.client.Response]: resolves with the HTTP response object on success. @@ -473,7 +479,8 @@ class MatrixFederationHttpClient(object): json_data_callback=None, long_retries=False, timeout=None, ignore_backoff=False, - backoff_on_404=False): + backoff_on_404=False, + try_trailing_slash_on_404=False): """ Sends the specifed json data using PUT Args: @@ -493,7 +500,11 @@ class MatrixFederationHttpClient(object): and try the request anyway. backoff_on_404 (bool): True if we should count a 404 response as a failure of the server (and should therefore back off future - requests) + requests). + try_trailing_slash_on_404 (bool): True if on a 404 response we + should try appending a trailing slash to the end of the + request. This will be attempted before backing off if backing + off has been enabled. Returns: Deferred[dict|list]: Succeeds when we get a 2xx HTTP response. The @@ -509,7 +520,6 @@ class MatrixFederationHttpClient(object): RequestSendFailed: If there were problems connecting to the remote, due to e.g. DNS failures, connection timeouts etc. """ - request = MatrixFederationRequest( method="PUT", destination=destination, @@ -519,13 +529,26 @@ class MatrixFederationHttpClient(object): json=data, ) - response = yield self._send_request( - request, - long_retries=long_retries, - timeout=timeout, - ignore_backoff=ignore_backoff, - backoff_on_404=backoff_on_404, - ) + 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 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_404 else backoff_on_404, + } + + response = yield self._send_request(**send_request_args) + + # If enabled, retry with a trailing slash if we received a 404 + if try_trailing_slash_on_404 and response.code == 404: + args["path"] += "/" + + # Re-enable backoff if enabled + send_request_args["backoff_on_404"] = backoff_on_404 + + response = yield self.get_json(**send_request_args) body = yield _handle_json_response( self.hs.get_reactor(), self.default_timeout, request, response, @@ -592,7 +615,8 @@ class MatrixFederationHttpClient(object): @defer.inlineCallbacks def get_json(self, destination, path, args=None, retry_on_dns_fail=True, - timeout=None, ignore_backoff=False): + timeout=None, ignore_backoff=False, + try_trailing_slash_on_404=False): """ GETs some json from the given host homeserver and path Args: @@ -606,6 +630,9 @@ class MatrixFederationHttpClient(object): be retried. ignore_backoff (bool): true to ignore the historical backoff data and try the request anyway. + try_trailing_slash_on_404 (bool): True if on a 404 response we + should try appending a trailing slash to the end of the + request. Returns: Deferred[dict|list]: Succeeds when we get a 2xx HTTP response. The result will be the decoded JSON body. @@ -631,63 +658,25 @@ class MatrixFederationHttpClient(object): query=args, ) - response = yield self._send_request( - request, - retry_on_dns_fail=retry_on_dns_fail, - timeout=timeout, - ignore_backoff=ignore_backoff, - ) + send_request_args = { + "request": request, + "retry_on_dns_fail": retry_on_dns_fail, + "timeout": timeout, + "ignore_backoff": ignore_backoff, + } + + response = yield self._send_request(**send_request_args) + + # If enabled, retry with a trailing slash if we received a 404 + if try_trailing_slash_on_404 and response.code == 404: + args["path"] += "/" + 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) - @defer.inlineCallbacks - def get_json_with_trailing_slashes_on_404(self, args={}): - """Runs client.get_json under the hood, but if receiving a 404, tries - the request again with a trailing slash. This is a result of removing - trailing slashes from some federation endpoints and in an effort to - remain backwards compatible with older versions of Synapse, we try - again if a server requires a trailing slash. - - Args: - args (dict): A dictionary of arguments matching those provided by put_json. - Returns: - Deferred[dict|list]: Succeeds when we get a 2xx HTTP response. The - result will be the decoded JSON body. - """ - response = yield self.get_json(**args) - - # Retry with a trailing slash if we received a 404 - if response.code == 404: - args["path"] += "/" - response = yield self.get_json(**args) - - defer.returnValue(response) - - @defer.inlineCallbacks - def put_json_with_trailing_slashes_on_404(self, args={}): - """Runs client.put_json under the hood, but if receiving a 404, tries - the request again with a trailing slash. - - See get_json_with_trailing_slashes_on_404 for more details. - - Args: - args (dict): A dictionary of arguments matching those provided by put_json. - Returns: - Deferred[dict|list]: Succeeds when we get a 2xx HTTP response. The - result will be the decoded JSON body. - """ - response = yield self.put_json(**args) - - # Retry with a trailing slash if we received a 404 - if response.code == 404: - args["path"] += "/" - response = yield self.put_json(**args) - - defer.returnValue(response) - @defer.inlineCallbacks def delete_json(self, destination, path, long_retries=False, timeout=None, ignore_backoff=False, args={}): diff --git a/tests/handlers/test_typing.py b/tests/handlers/test_typing.py index 5a3670c93..e17abf2fb 100644 --- a/tests/handlers/test_typing.py +++ b/tests/handlers/test_typing.py @@ -177,7 +177,7 @@ class TypingNotificationsTestCase(unittest.HomeserverTestCase): timeout=20000, )) - put_json = self.hs.get_http_client().put_json_with_trailing_slashes_on_404 + put_json = self.hs.get_http_client().put_json put_json.assert_called_once_with( "farm", path="/_matrix/federation/v1/send/1000000", @@ -192,6 +192,7 @@ class TypingNotificationsTestCase(unittest.HomeserverTestCase): json_data_callback=ANY, long_retries=True, backoff_on_404=True, + trailing_slashes_on_404=True, ) def test_started_typing_remote_recv(self): @@ -254,7 +255,7 @@ class TypingNotificationsTestCase(unittest.HomeserverTestCase): [call('typing_key', 1, rooms=[ROOM_ID])] ) - put_json = self.hs.get_http_client().put_json_with_trailing_slashes_on_404 + put_json = self.hs.get_http_client().put_json put_json.assert_called_once_with( "farm", path="/_matrix/federation/v1/send/1000000", @@ -269,6 +270,7 @@ class TypingNotificationsTestCase(unittest.HomeserverTestCase): json_data_callback=ANY, long_retries=True, backoff_on_404=True, + trailing_slashes_on_404=True, ) self.assertEquals(self.event_source.get_current_key(), 1) From 97653ef1f4486259cdb4c1a39bd5f43522311691 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 12 Mar 2019 14:30:26 +0000 Subject: [PATCH 09/40] Correct argument name --- tests/handlers/test_typing.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/handlers/test_typing.py b/tests/handlers/test_typing.py index e17abf2fb..244a0bc80 100644 --- a/tests/handlers/test_typing.py +++ b/tests/handlers/test_typing.py @@ -192,7 +192,7 @@ class TypingNotificationsTestCase(unittest.HomeserverTestCase): json_data_callback=ANY, long_retries=True, backoff_on_404=True, - trailing_slashes_on_404=True, + try_trailing_slash_on_404=True, ) def test_started_typing_remote_recv(self): @@ -270,7 +270,7 @@ class TypingNotificationsTestCase(unittest.HomeserverTestCase): json_data_callback=ANY, long_retries=True, backoff_on_404=True, - trailing_slashes_on_404=True, + try_trailing_slash_on_404=True, ) self.assertEquals(self.event_source.get_current_key(), 1) From cf301e37d873fcc00a9fb2d5efffd8ff7ad1b372 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 11:14:43 +0000 Subject: [PATCH 10/40] Add workaround note --- synapse/http/matrixfederationclient.py | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index fec7f5f88..d22051a47 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -197,7 +197,6 @@ class MatrixFederationHttpClient(object): long_retries=False, ignore_backoff=False, backoff_on_404=False, - try_trailing_slash_on_404=False, ): """ Sends a request to the given server. @@ -213,11 +212,6 @@ class MatrixFederationHttpClient(object): backoff_on_404 (bool): Back off if we get a 404 - try_trailing_slash_on_404 (bool): True if on a 404 response we - should try appending a trailing slash to the end of the - request. This will be attempted before backing off if backing - off has been enabled. - Returns: Deferred[twisted.web.client.Response]: resolves with the HTTP response object on success. @@ -502,9 +496,9 @@ class MatrixFederationHttpClient(object): a failure of the server (and should therefore back off future requests). try_trailing_slash_on_404 (bool): True if on a 404 response we - should try appending a trailing slash to the end of the - request. This will be attempted before backing off if backing - off has been enabled. + should try appending a trailing slash to the end of the request. + Workaround for #3622 in Synapse <0.99.2. This will be attempted + before backing off if backing off has been enabled. Returns: Deferred[dict|list]: Succeeds when we get a 2xx HTTP response. The @@ -632,7 +626,7 @@ class MatrixFederationHttpClient(object): and try the request anyway. try_trailing_slash_on_404 (bool): True if on a 404 response we should try appending a trailing slash to the end of the - request. + request. Workaround for #3622 in Synapse <0.99.2. Returns: Deferred[dict|list]: Succeeds when we get a 2xx HTTP response. The result will be the decoded JSON body. From 7e75d9644bfb5d687c3b789ad2279da58e09ed7b Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 11:15:23 +0000 Subject: [PATCH 11/40] Fix paranthesis indent --- synapse/http/matrixfederationclient.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index d22051a47..7efa7b757 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -474,7 +474,8 @@ class MatrixFederationHttpClient(object): long_retries=False, timeout=None, ignore_backoff=False, backoff_on_404=False, - try_trailing_slash_on_404=False): + try_trailing_slash_on_404=False, + ): """ Sends the specifed json data using PUT Args: From 7d053cfe10c7be4327b774c6552d21594dcdd2be Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 12:10:33 +0000 Subject: [PATCH 12/40] Retry on 400:M_UNRECOGNIZED --- synapse/http/matrixfederationclient.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 7efa7b757..fc8cf9206 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -474,8 +474,7 @@ class MatrixFederationHttpClient(object): long_retries=False, timeout=None, ignore_backoff=False, backoff_on_404=False, - try_trailing_slash_on_404=False, - ): + try_trailing_slash_on_404=False): """ Sends the specifed json data using PUT Args: @@ -662,14 +661,19 @@ class MatrixFederationHttpClient(object): response = yield self._send_request(**send_request_args) - # If enabled, retry with a trailing slash if we received a 404 - if try_trailing_slash_on_404 and response.code == 404: - args["path"] += "/" - response = yield self._send_request(**send_request_args) - body = yield _handle_json_response( self.hs.get_reactor(), self.default_timeout, request, response, ) + + # If enabled, retry with a trailing slash if we received a 404 + # or if a 400 with "M_UNRECOGNIZED" which some endpoints return + if (try_trailing_slash_on_404 and + (response.code == 404 + or (response.code == 400 + and body.get("errcode") == "M_UNRECOGNIZED"))): + args["path"] += "/" + response = yield self._send_request(**send_request_args) + defer.returnValue(body) @defer.inlineCallbacks From 09626bfd39615e25c735ae5d17ad650aca5fac84 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 18:26:06 +0000 Subject: [PATCH 13/40] Switch to wrapper function around _send_request --- synapse/federation/transport/client.py | 10 +-- synapse/http/matrixfederationclient.py | 103 +++++++++++++++++-------- tests/handlers/test_typing.py | 4 +- 3 files changed, 78 insertions(+), 39 deletions(-) diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py index 37f37b3d4..e424c40fd 100644 --- a/synapse/federation/transport/client.py +++ b/synapse/federation/transport/client.py @@ -54,7 +54,7 @@ class TransportLayerClient(object): path = _create_v1_path("/state/%s", room_id) return self.client.get_json( destination, path=path, args={"event_id": event_id}, - try_trailing_slash_on_404=True, + try_trailing_slash_on_400=True, ) @log_function @@ -77,7 +77,7 @@ class TransportLayerClient(object): path = _create_v1_path("/state_ids/%s", room_id) return self.client.get_json( destination, path=path, args={"event_id": event_id}, - try_trailing_slash_on_404=True, + try_trailing_slash_on_400=True, ) @log_function @@ -100,7 +100,7 @@ class TransportLayerClient(object): path = _create_v1_path("/event/%s", event_id) return self.client.get_json( destination, path=path, timeout=timeout, - try_trailing_slash_on_404=True, + try_trailing_slash_on_400=True, ) @log_function @@ -137,7 +137,7 @@ class TransportLayerClient(object): destination, path=path, args=args, - try_trailing_slash_on_404=True, + try_trailing_slash_on_400=True, ) @defer.inlineCallbacks @@ -182,7 +182,7 @@ class TransportLayerClient(object): json_data_callback=json_data_callback, long_retries=True, backoff_on_404=True, # If we get a 404 the other side has gone - try_trailing_slash_on_404=True, + try_trailing_slash_on_400=True, ) defer.returnValue(response) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index fc8cf9206..9019c8791 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -188,6 +188,55 @@ class MatrixFederationHttpClient(object): self._cooperator = Cooperator(scheduler=schedule) + @defer.inlineCallbacks + def _send_request_with_optional_trailing_slash( + request, + try_trailing_slash_on_400=False, + backoff_on_404=False, + **kwargs, + ): + """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 + due to #3622. + + Args: + request + 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. + backoff_on_404 (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). + kwargs (Dict): A dictionary of arguments to pass to + `_send_request()`. + + Returns: + Deferred[twisted.web.client.Response]: resolves with the HTTP + response object on success. + """ + response = self._send_request(**kwargs) + + if not try_trailing_slash_on_400: + defer.returnValue(response) + + # 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, + ) + + # 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 (response.code == 400 and body.get("errcode") == "M_UNRECOGNIZED"): + # Enable backoff if initially disabled + kwargs["backoff_on_404"] = backoff_on_404 + + kwargs["path"] += "/" + response = yield self._send_request(**kwargs) + + defer.returnValue(response) + @defer.inlineCallbacks def _send_request( self, @@ -474,7 +523,7 @@ class MatrixFederationHttpClient(object): long_retries=False, timeout=None, ignore_backoff=False, backoff_on_404=False, - try_trailing_slash_on_404=False): + try_trailing_slash_on_400=False): """ Sends the specifed json data using PUT Args: @@ -495,10 +544,11 @@ class MatrixFederationHttpClient(object): backoff_on_404 (bool): True if we should count a 404 response as a failure of the server (and should therefore back off future requests). - try_trailing_slash_on_404 (bool): True if on a 404 response we - should try appending a trailing slash to the end of the request. - Workaround for #3622 in Synapse <0.99.2. This will be attempted - before backing off if backing off has been enabled. + 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 + will be attempted before backing off if backing off has been + enabled. Returns: Deferred[dict|list]: Succeeds when we get a 2xx HTTP response. The @@ -528,21 +578,21 @@ class MatrixFederationHttpClient(object): "long_retries": long_retries, "timeout": timeout, "ignore_backoff": ignore_backoff, - # Do not backoff on the initial request if we're trying 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_404 else backoff_on_404, + # 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(**send_request_args) + response = yield self._send_request_with_optional_trailing_slash( + request, try_trailing_slash_on_400, backoff_on_404, **send_request_args, + ) - # If enabled, retry with a trailing slash if we received a 404 - if try_trailing_slash_on_404 and response.code == 404: + # If enabled, retry with a trailing slash if we received a 400 + if try_trailing_slash_on_400 and response.code == 400: args["path"] += "/" - # Re-enable backoff if enabled - send_request_args["backoff_on_404"] = backoff_on_404 - - response = yield self.get_json(**send_request_args) + response = yield self._send_request(**send_request_args) body = yield _handle_json_response( self.hs.get_reactor(), self.default_timeout, request, response, @@ -610,7 +660,7 @@ class MatrixFederationHttpClient(object): @defer.inlineCallbacks def get_json(self, destination, path, args=None, retry_on_dns_fail=True, timeout=None, ignore_backoff=False, - try_trailing_slash_on_404=False): + try_trailing_slash_on_400=False): """ GETs some json from the given host homeserver and path Args: @@ -624,9 +674,9 @@ class MatrixFederationHttpClient(object): be retried. ignore_backoff (bool): true to ignore the historical backoff data and try the request anyway. - try_trailing_slash_on_404 (bool): True if on a 404 response we - should try appending a trailing slash to the end of the - request. Workaround for #3622 in Synapse <0.99.2. + 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. Returns: Deferred[dict|list]: Succeeds when we get a 2xx HTTP response. The result will be the decoded JSON body. @@ -659,21 +709,10 @@ class MatrixFederationHttpClient(object): "ignore_backoff": ignore_backoff, } - 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_with_optional_trailing_slash( + request, try_trailing_slash_on_400, False, **send_request_args, ) - # If enabled, retry with a trailing slash if we received a 404 - # or if a 400 with "M_UNRECOGNIZED" which some endpoints return - if (try_trailing_slash_on_404 and - (response.code == 404 - or (response.code == 400 - and body.get("errcode") == "M_UNRECOGNIZED"))): - args["path"] += "/" - response = yield self._send_request(**send_request_args) - defer.returnValue(body) @defer.inlineCallbacks diff --git a/tests/handlers/test_typing.py b/tests/handlers/test_typing.py index 244a0bc80..6460cbc70 100644 --- a/tests/handlers/test_typing.py +++ b/tests/handlers/test_typing.py @@ -192,7 +192,7 @@ class TypingNotificationsTestCase(unittest.HomeserverTestCase): json_data_callback=ANY, long_retries=True, backoff_on_404=True, - try_trailing_slash_on_404=True, + try_trailing_slash_on_400=True, ) def test_started_typing_remote_recv(self): @@ -270,7 +270,7 @@ class TypingNotificationsTestCase(unittest.HomeserverTestCase): json_data_callback=ANY, long_retries=True, backoff_on_404=True, - try_trailing_slash_on_404=True, + try_trailing_slash_on_400=True, ) self.assertEquals(self.event_source.get_current_key(), 1) From 5526b054aaf4ca4c0b459a19019d41a0cd3a1978 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 18:35:21 +0000 Subject: [PATCH 14/40] Fix syntax issues --- synapse/http/matrixfederationclient.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 9019c8791..5b89a2e05 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -190,6 +190,7 @@ class MatrixFederationHttpClient(object): @defer.inlineCallbacks def _send_request_with_optional_trailing_slash( + self, request, try_trailing_slash_on_400=False, backoff_on_404=False, @@ -215,7 +216,7 @@ class MatrixFederationHttpClient(object): Deferred[twisted.web.client.Response]: resolves with the HTTP response object on success. """ - response = self._send_request(**kwargs) + response = yield self._send_request(**kwargs) if not try_trailing_slash_on_400: defer.returnValue(response) @@ -225,6 +226,9 @@ class MatrixFederationHttpClient(object): self.hs.get_reactor(), self.default_timeout, request, response, ) + logger.info(" *** BODY IS *** ") + logger.info(body) + # 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. @@ -588,15 +592,10 @@ class MatrixFederationHttpClient(object): request, try_trailing_slash_on_400, backoff_on_404, **send_request_args, ) - # If enabled, retry with a trailing slash if we received a 400 - if try_trailing_slash_on_400 and response.code == 400: - args["path"] += "/" - - 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) @defer.inlineCallbacks @@ -713,6 +712,10 @@ class MatrixFederationHttpClient(object): 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) @defer.inlineCallbacks From 660b77f3626583fcc49a76e5c2b3d1143677799b Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 18:38:16 +0000 Subject: [PATCH 15/40] Add missing docstring detail --- synapse/http/matrixfederationclient.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 5b89a2e05..fca6e242b 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -202,7 +202,7 @@ class MatrixFederationHttpClient(object): due to #3622. Args: - request + request (MatrixFederationRequest): details of request to be sent 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. From 220607a6183a60a62cfa3fbcfcd30fadc0bdff4b Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 18:43:40 +0000 Subject: [PATCH 16/40] Remove testing code --- synapse/http/matrixfederationclient.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index fca6e242b..380bf294e 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -226,9 +226,6 @@ class MatrixFederationHttpClient(object): self.hs.get_reactor(), self.default_timeout, request, response, ) - logger.info(" *** BODY IS *** ") - logger.info(body) - # 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. From 9dd0e34679f711116f39a35466e7dc956b10ab42 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 18:45:17 +0000 Subject: [PATCH 17/40] Syntax test --- synapse/http/matrixfederationclient.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 380bf294e..cdeb3792b 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -190,12 +190,11 @@ class MatrixFederationHttpClient(object): @defer.inlineCallbacks def _send_request_with_optional_trailing_slash( - self, - request, - try_trailing_slash_on_400=False, - backoff_on_404=False, - **kwargs, - ): + self, + request, + try_trailing_slash_on_400=False, + backoff_on_404=False, + **kwargs): """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 From ee8ba397e89d61ec7beeb549391b9ffeaf7e484a Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 18:48:31 +0000 Subject: [PATCH 18/40] Are you happy now --- synapse/http/matrixfederationclient.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index cdeb3792b..f554d5e21 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -585,7 +585,7 @@ class MatrixFederationHttpClient(object): } response = yield self._send_request_with_optional_trailing_slash( - request, try_trailing_slash_on_400, backoff_on_404, **send_request_args, + request, try_trailing_slash_on_400, backoff_on_404, send_request_args, ) body = yield _handle_json_response( @@ -705,7 +705,7 @@ class MatrixFederationHttpClient(object): } response = yield self._send_request_with_optional_trailing_slash( - request, try_trailing_slash_on_400, False, **send_request_args, + request, try_trailing_slash_on_400, False, send_request_args, ) body = yield _handle_json_response( From c2d848b80d8b83646b1a0413057efad910a3df12 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 19:04:43 +0000 Subject: [PATCH 19/40] Destructure again --- synapse/http/matrixfederationclient.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index f554d5e21..380bf294e 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -190,11 +190,12 @@ class MatrixFederationHttpClient(object): @defer.inlineCallbacks def _send_request_with_optional_trailing_slash( - self, - request, - try_trailing_slash_on_400=False, - backoff_on_404=False, - **kwargs): + self, + request, + try_trailing_slash_on_400=False, + backoff_on_404=False, + **kwargs, + ): """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 @@ -585,7 +586,7 @@ class MatrixFederationHttpClient(object): } response = yield self._send_request_with_optional_trailing_slash( - request, try_trailing_slash_on_400, backoff_on_404, send_request_args, + request, try_trailing_slash_on_400, backoff_on_404, **send_request_args, ) body = yield _handle_json_response( @@ -705,7 +706,7 @@ class MatrixFederationHttpClient(object): } response = yield self._send_request_with_optional_trailing_slash( - request, try_trailing_slash_on_400, False, send_request_args, + request, try_trailing_slash_on_400, False, **send_request_args, ) body = yield _handle_json_response( From c991e7aec78d7cb299c4f9c16f3b80f03c887bfb Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 19:08:08 +0000 Subject: [PATCH 20/40] Syntax checker is bork --- synapse/http/matrixfederationclient.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 380bf294e..cdeb3792b 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -190,12 +190,11 @@ class MatrixFederationHttpClient(object): @defer.inlineCallbacks def _send_request_with_optional_trailing_slash( - self, - request, - try_trailing_slash_on_400=False, - backoff_on_404=False, - **kwargs, - ): + self, + request, + try_trailing_slash_on_400=False, + backoff_on_404=False, + **kwargs): """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 From bec313818c03f89b501fb8b07efa577f47efee8e Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 19:10:56 +0000 Subject: [PATCH 21/40] go home python, you're drunk --- synapse/http/matrixfederationclient.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index cdeb3792b..de6bc4de0 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -585,8 +585,7 @@ class MatrixFederationHttpClient(object): } response = yield self._send_request_with_optional_trailing_slash( - request, try_trailing_slash_on_400, backoff_on_404, **send_request_args, - ) + 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, @@ -705,8 +704,7 @@ class MatrixFederationHttpClient(object): } response = yield self._send_request_with_optional_trailing_slash( - request, try_trailing_slash_on_400, False, **send_request_args, - ) + request, try_trailing_slash_on_400, False, **send_request_args) body = yield _handle_json_response( self.hs.get_reactor(), self.default_timeout, request, response, From 66cdb840a6fdf24431cce50e776d65290c419c86 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 19:18:25 +0000 Subject: [PATCH 22/40] Or perhaps I was the one who was drunk --- synapse/http/matrixfederationclient.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index de6bc4de0..f596ccfdf 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -193,7 +193,6 @@ class MatrixFederationHttpClient(object): self, request, try_trailing_slash_on_400=False, - backoff_on_404=False, **kwargs): """Wrapper for _send_request which can optionally retry the request upon receiving a combination of a 400 HTTP response code and a @@ -205,9 +204,6 @@ 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. - backoff_on_404 (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). kwargs (Dict): A dictionary of arguments to pass to `_send_request()`. @@ -585,7 +581,7 @@ class MatrixFederationHttpClient(object): } response = yield self._send_request_with_optional_trailing_slash( - request, try_trailing_slash_on_400, backoff_on_404, **send_request_args) + request, try_trailing_slash_on_400, **send_request_args) body = yield _handle_json_response( self.hs.get_reactor(), self.default_timeout, request, response, @@ -701,10 +697,11 @@ class MatrixFederationHttpClient(object): "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, **send_request_args) + request, try_trailing_slash_on_400, **send_request_args) body = yield _handle_json_response( self.hs.get_reactor(), self.default_timeout, request, response, From 7c0295f13c00ab1cc613584214af1bfc98041edd Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 19:27:10 +0000 Subject: [PATCH 23/40] no kwargs today --- synapse/http/matrixfederationclient.py | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index f596ccfdf..47c7aedff 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -193,7 +193,9 @@ class MatrixFederationHttpClient(object): self, request, try_trailing_slash_on_400=False, - **kwargs): + backoff_on_404=False, + 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 'M_UNRECOGNIZED' errcode. This is a workaround for Synapse <=v0.99.2 @@ -204,14 +206,17 @@ 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. - kwargs (Dict): A dictionary of arguments to pass to + 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). + send_request_args (Dict): A dictionary of arguments to pass to `_send_request()`. Returns: Deferred[twisted.web.client.Response]: resolves with the HTTP response object on success. """ - response = yield self._send_request(**kwargs) + response = yield self._send_request(**send_request_args) if not try_trailing_slash_on_400: defer.returnValue(response) @@ -226,10 +231,10 @@ class MatrixFederationHttpClient(object): # trailing slash on Synapse <=v0.99.2. if (response.code == 400 and body.get("errcode") == "M_UNRECOGNIZED"): # Enable backoff if initially disabled - kwargs["backoff_on_404"] = backoff_on_404 + send_request_args["backoff_on_404"] = backoff_on_404 - kwargs["path"] += "/" - response = yield self._send_request(**kwargs) + send_request_args["path"] += "/" + response = yield self._send_request(**send_request_args) defer.returnValue(response) @@ -581,7 +586,7 @@ class MatrixFederationHttpClient(object): } response = yield self._send_request_with_optional_trailing_slash( - request, try_trailing_slash_on_400, **send_request_args) + 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, From 5ca857ad849784ce72660fce8d8a94bb4aa55d5f Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 19:35:23 +0000 Subject: [PATCH 24/40] as above --- synapse/http/matrixfederationclient.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 47c7aedff..502ff1512 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -586,7 +586,8 @@ class MatrixFederationHttpClient(object): } response = yield self._send_request_with_optional_trailing_slash( - request, try_trailing_slash_on_400, backoff_on_404, send_request_args) + 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, @@ -706,7 +707,8 @@ class MatrixFederationHttpClient(object): } response = yield self._send_request_with_optional_trailing_slash( - request, try_trailing_slash_on_400, **send_request_args) + request, try_trailing_slash_on_400, send_request_args, + ) body = yield _handle_json_response( self.hs.get_reactor(), self.default_timeout, request, response, From 26f8e2d099e59a20abf56895b7958f5af853db32 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 19:49:08 +0000 Subject: [PATCH 25/40] there comes a time when you should give up. but you dont --- synapse/http/matrixfederationclient.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 502ff1512..269cf2148 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -707,7 +707,7 @@ class MatrixFederationHttpClient(object): } response = yield self._send_request_with_optional_trailing_slash( - request, try_trailing_slash_on_400, send_request_args, + request, try_trailing_slash_on_400, backoff_on_404, send_request_args, ) body = yield _handle_json_response( From 8d16ffaf7a1ab9ae17e59b2d80b82aa65294bf95 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 20:03:10 +0000 Subject: [PATCH 26/40] i should have given up --- synapse/http/matrixfederationclient.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 269cf2148..8ad28d1e2 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -707,7 +707,7 @@ class MatrixFederationHttpClient(object): } response = yield self._send_request_with_optional_trailing_slash( - request, try_trailing_slash_on_400, backoff_on_404, send_request_args, + request, try_trailing_slash_on_400, False, send_request_args, ) body = yield _handle_json_response( From 45524f2f5e9dc7eca5c439e4a8bff938abc17ee8 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 20:17:39 +0000 Subject: [PATCH 27/40] i should have given up x2 --- synapse/http/matrixfederationclient.py | 31 +++++++++----------- tests/http/test_fedclient.py | 39 ++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 18 deletions(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 8ad28d1e2..74ea6bcf8 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -190,11 +190,11 @@ class MatrixFederationHttpClient(object): @defer.inlineCallbacks def _send_request_with_optional_trailing_slash( - self, - request, - try_trailing_slash_on_400=False, - backoff_on_404=False, - send_request_args={}, + self, + request, + try_trailing_slash_on_400=False, + backoff_on_404=False, + 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 @@ -213,8 +213,7 @@ class MatrixFederationHttpClient(object): `_send_request()`. Returns: - Deferred[twisted.web.client.Response]: resolves with the HTTP - response object on success. + Deferred[Dict]: Parsed JSON response body. """ response = yield self._send_request(**send_request_args) @@ -236,7 +235,11 @@ class MatrixFederationHttpClient(object): send_request_args["path"] += "/" response = yield self._send_request(**send_request_args) - defer.returnValue(response) + body = yield _handle_json_response( + self.hs.get_reactor(), self.default_timeout, request, response, + ) + + defer.returnValue(body) @defer.inlineCallbacks def _send_request( @@ -585,14 +588,10 @@ class MatrixFederationHttpClient(object): "backoff_on_404": False if try_trailing_slash_on_400 else backoff_on_404, } - response = yield self._send_request_with_optional_trailing_slash( + 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) @defer.inlineCallbacks @@ -706,14 +705,10 @@ class MatrixFederationHttpClient(object): "backoff_on_404": False, } - response = yield self._send_request_with_optional_trailing_slash( + 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) @defer.inlineCallbacks diff --git a/tests/http/test_fedclient.py b/tests/http/test_fedclient.py index b03b37aff..0d0161d13 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, From 86c60bda15d806cf4f6700dee2878de762cd240c Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 20:19:07 +0000 Subject: [PATCH 28/40] i should have given up x3 --- tests/http/test_fedclient.py | 39 ------------------------------------ 1 file changed, 39 deletions(-) diff --git a/tests/http/test_fedclient.py b/tests/http/test_fedclient.py index 0d0161d13..b03b37aff 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, From 9a2e22fd41845b6fa7f30c66cf89a368af269147 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 20:29:38 +0000 Subject: [PATCH 29/40] is this what purgatory feels like --- synapse/http/matrixfederationclient.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 74ea6bcf8..59e758fef 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -217,14 +217,14 @@ class MatrixFederationHttpClient(object): """ response = yield self._send_request(**send_request_args) - if not try_trailing_slash_on_400: - defer.returnValue(response) - # 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, ) + if not try_trailing_slash_on_400: + defer.returnValue(body) + # 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. From b2df0e8e2cb72a2147e6d962a0e8c5ce35233ed4 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 21:08:10 +0000 Subject: [PATCH 30/40] receiving a 400 caused an exception. handle it --- synapse/http/matrixfederationclient.py | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 59e758fef..6b4163974 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -212,27 +212,34 @@ class MatrixFederationHttpClient(object): send_request_args (Dict): A dictionary of arguments to pass to `_send_request()`. + Raises: + HttpResponseException: If we get an HTTP response code >= 300 + (except 429). + Returns: Deferred[Dict]: Parsed JSON response body. """ - response = yield self._send_request(**send_request_args) + try: + response = yield self._send_request(**send_request_args) + except HttpResponseException as e: + # Received a 400. Raise unless we're retrying + if not try_trailing_slash_on_400: + 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, ) - if not try_trailing_slash_on_400: - defer.returnValue(body) - # 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 (response.code == 400 and body.get("errcode") == "M_UNRECOGNIZED"): + 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 send_request_args["path"] += "/" + response = yield self._send_request(**send_request_args) body = yield _handle_json_response( From ecea5af491cd69933afe1dbea665d85c84d1d94e Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 21:21:03 +0000 Subject: [PATCH 31/40] Correct var name --- synapse/http/matrixfederationclient.py | 4 +- tests/http/test_fedclient.py | 54 ++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 6b4163974..b27c4c1c3 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 b03b37aff..b45eee3a8 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, From 621e7f37f1a7f32ff5046060f17a1da825f9ff8b Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 18 Mar 2019 17:45:54 +0000 Subject: [PATCH 32/40] Better exception handling --- synapse/http/matrixfederationclient.py | 41 +++++++++++++------------- tests/http/test_fedclient.py | 5 +--- 2 files changed, 22 insertions(+), 24 deletions(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index b27c4c1c3..3c27686a8 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) - except HttpResponseException as e: - # Received a 400. Raise unless we're retrying - if not try_trailing_slash_on_400: - 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, - ) - - # 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 - - # Add trailing slash - send_request_args["request"].path += "/" - - 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: + if not try_trailing_slash_on_400: + # Received an error >= 300. Raise unless we're retrying + raise e + 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. + # Enable backoff if initially disabled + send_request_args["backoff_on_404"] = backoff_on_404 + + # 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, + ) defer.returnValue(body) diff --git a/tests/http/test_fedclient.py b/tests/http/test_fedclient.py index b45eee3a8..84216db44 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/") From a8ad39eec74ff973f6bfe8d84dfd8d2d3ca32913 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 18 Mar 2019 17:47:39 +0000 Subject: [PATCH 33/40] lint --- synapse/http/matrixfederationclient.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 3c27686a8..2be024487 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -230,7 +230,7 @@ class MatrixFederationHttpClient(object): if not try_trailing_slash_on_400: # Received an error >= 300. Raise unless we're retrying raise e - except: + except Exception as e: raise e # Retry with a trailing slash if we received a 400 with From 94cb7939e4d608500396b65d5c68dd936503ba69 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 20 Mar 2019 10:50:44 +0000 Subject: [PATCH 34/40] Federation test fixed! --- tests/http/test_fedclient.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/http/test_fedclient.py b/tests/http/test_fedclient.py index 84216db44..b1b3a025e 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): From 551ea1155966c023d7d2eb41454598bf14cb4be2 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 20 Mar 2019 11:07:36 +0000 Subject: [PATCH 35/40] Just return if not doing any trailing slash shennanigans --- synapse/http/matrixfederationclient.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 2be024487..1307508e5 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -226,6 +226,8 @@ class MatrixFederationHttpClient(object): body = yield _handle_json_response( self.hs.get_reactor(), self.default_timeout, request, response, ) + + defer.returnValue(body) except HttpResponseException as e: if not try_trailing_slash_on_400: # Received an error >= 300. Raise unless we're retrying From c69df5d5d37652b98d670a25c8c4291002af9242 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 20 Mar 2019 11:27:18 +0000 Subject: [PATCH 36/40] 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(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 1307508e5..8312e4fc3 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 b1b3a025e..de5da1694 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, From cd36a1283b6036505b77f51a0cc860e8e5a0878d Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 20 Mar 2019 14:00:39 +0000 Subject: [PATCH 37/40] New test, fix issues --- synapse/http/matrixfederationclient.py | 77 ++++++++++---------------- tests/http/test_fedclient.py | 45 +++++++++++++++ 2 files changed, 73 insertions(+), 49 deletions(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 8312e4fc3..dd358536f 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 de5da1694..fbe87d4d0 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, From bb52a2e65308f10285d5d6290786e889d203f3db Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 20 Mar 2019 14:08:57 +0000 Subject: [PATCH 38/40] lint --- synapse/http/matrixfederationclient.py | 8 ++++++-- tests/http/test_fedclient.py | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index dd358536f..2c1bcf66f 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 fbe87d4d0..cd8e086f8 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( From 2150151abe919884671fe2d080e5145d9face5fa Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 20 Mar 2019 14:13:32 +0000 Subject: [PATCH 39/40] kwargs doesn't like commas on calling funcs either. TIL --- synapse/http/matrixfederationclient.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 2c1bcf66f..c654c1cf1 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -220,7 +220,7 @@ class MatrixFederationHttpClient(object): """ try: response = yield self._send_request( - request, backoff_on_404=backoff_on_404, **send_request_args, + 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 @@ -237,7 +237,7 @@ class MatrixFederationHttpClient(object): request.path += "/" response = yield self._send_request( - request, backoff_on_404=backoff_on_404, **send_request_args, + request, backoff_on_404=backoff_on_404, **send_request_args ) defer.returnValue(response) From b41c2eaadc1b9d6150ac0f7979e877e5ee9ab80e Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Thu, 21 Mar 2019 14:32:47 +0000 Subject: [PATCH 40/40] Clean up backoff_on_404 and metehod calls --- synapse/http/matrixfederationclient.py | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index c654c1cf1..8e855d13d 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -193,7 +193,6 @@ class MatrixFederationHttpClient(object): self, request, try_trailing_slash_on_400=False, - backoff_on_404=False, **send_request_args ): """Wrapper for _send_request which can optionally retry the request @@ -206,8 +205,6 @@ 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. - 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,7 +217,7 @@ class MatrixFederationHttpClient(object): """ try: response = yield self._send_request( - request, backoff_on_404=backoff_on_404, **send_request_args + request, **send_request_args ) except HttpResponseException as e: # Received an HTTP error > 300. Check if it meets the requirements @@ -237,7 +234,7 @@ class MatrixFederationHttpClient(object): request.path += "/" response = yield self._send_request( - request, backoff_on_404=backoff_on_404, **send_request_args + request, **send_request_args ) defer.returnValue(response) @@ -579,8 +576,12 @@ class MatrixFederationHttpClient(object): ) 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, + request, + try_trailing_slash_on_400, + backoff_on_404=backoff_on_404, + ignore_backoff=ignore_backoff, + long_retries=long_retries, + timeout=timeout, ) body = yield _handle_json_response( @@ -693,9 +694,12 @@ class MatrixFederationHttpClient(object): ) 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, + request, + try_trailing_slash_on_400, + backoff_on_404=False, ignore_backoff=ignore_backoff, + retry_on_dns_fail=retry_on_dns_fail, + timeout=timeout, ) body = yield _handle_json_response(