From ecd7e36047d090cdb027f500b0f95a375ba61811 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Mon, 13 Feb 2017 13:16:48 +0000 Subject: [PATCH 1/4] http txns: Do not cache error responses Previously we did. This meant that, amongst other errors, rate-limiting errors would be cached and prevent messages with that txn ID being sent. --- synapse/rest/client/transactions.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/synapse/rest/client/transactions.py b/synapse/rest/client/transactions.py index efa77b8c5..6396a0803 100644 --- a/synapse/rest/client/transactions.py +++ b/synapse/rest/client/transactions.py @@ -81,7 +81,16 @@ class HttpTransactionCache(object): Deferred which resolves to a tuple of (response_code, response_dict). """ try: - return self.transactions[txn_key][0].observe() + observable = self.transactions[txn_key][0] + if not observable.has_called() or observable.has_succeeded(): + return observable.observe() + # if the request has already been called with a non-2xx status + # (a Twisted failure), remove it from the transaction map. + # This is done to ensure that we don't cache rate-limiting errors, etc. + res = observable.get_result() + if res.value.code >= 300: + del self.transactions[txn_key] + # fall through except (KeyError, IndexError): pass # execute the function instead. From feb15dc99f02e6cb0a84a53e397529c51743f114 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Mon, 13 Feb 2017 13:33:12 +0000 Subject: [PATCH 2/4] Don't cache errors at all --- synapse/rest/client/transactions.py | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/synapse/rest/client/transactions.py b/synapse/rest/client/transactions.py index 6396a0803..95376a2fb 100644 --- a/synapse/rest/client/transactions.py +++ b/synapse/rest/client/transactions.py @@ -81,16 +81,7 @@ class HttpTransactionCache(object): Deferred which resolves to a tuple of (response_code, response_dict). """ try: - observable = self.transactions[txn_key][0] - if not observable.has_called() or observable.has_succeeded(): - return observable.observe() - # if the request has already been called with a non-2xx status - # (a Twisted failure), remove it from the transaction map. - # This is done to ensure that we don't cache rate-limiting errors, etc. - res = observable.get_result() - if res.value.code >= 300: - del self.transactions[txn_key] - # fall through + return self.transactions[txn_key][0].observe() except (KeyError, IndexError): pass # execute the function instead. @@ -101,6 +92,14 @@ class HttpTransactionCache(object): # to the observers. observable = ObservableDeferred(deferred, consumeErrors=True) self.transactions[txn_key] = (observable, self.clock.time_msec()) + + # if the request fails with a Twisted failure, remove it + # from the transaction map. This is done to ensure that we don't + # cache transient errors like rate-limiting errors, etc. + def remove_from_map(err): + del self.transactions[txn_key] + return err + observable.addErrback(remove_from_map) return observable.observe() def _cleanup(self): From 808ddf0ae72ef45d887e00c07ba834d0873ceb8d Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Mon, 13 Feb 2017 13:36:15 +0000 Subject: [PATCH 3/4] Pop the txn from the map in case it has already been deleted somehow --- synapse/rest/client/transactions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/rest/client/transactions.py b/synapse/rest/client/transactions.py index 95376a2fb..7b742ca7a 100644 --- a/synapse/rest/client/transactions.py +++ b/synapse/rest/client/transactions.py @@ -97,7 +97,7 @@ class HttpTransactionCache(object): # from the transaction map. This is done to ensure that we don't # cache transient errors like rate-limiting errors, etc. def remove_from_map(err): - del self.transactions[txn_key] + self.transactions.pop(txn_key, None) return err observable.addErrback(remove_from_map) return observable.observe() From d0497425f81ed21b58046f5f8725fc70ffcc0544 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Mon, 13 Feb 2017 13:49:44 +0000 Subject: [PATCH 4/4] Ordering is important on errbacks so add the cleanup func before creating an ObservableDeferred --- synapse/rest/client/transactions.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/synapse/rest/client/transactions.py b/synapse/rest/client/transactions.py index 7b742ca7a..fceca2ede 100644 --- a/synapse/rest/client/transactions.py +++ b/synapse/rest/client/transactions.py @@ -87,19 +87,19 @@ class HttpTransactionCache(object): deferred = fn(*args, **kwargs) - # We don't add an errback to the raw deferred, so we ask ObservableDeferred - # to swallow the error. This is fine as the error will still be reported - # to the observers. - observable = ObservableDeferred(deferred, consumeErrors=True) - self.transactions[txn_key] = (observable, self.clock.time_msec()) - # if the request fails with a Twisted failure, remove it # from the transaction map. This is done to ensure that we don't # cache transient errors like rate-limiting errors, etc. def remove_from_map(err): self.transactions.pop(txn_key, None) return err - observable.addErrback(remove_from_map) + deferred.addErrback(remove_from_map) + + # We don't add any other errbacks to the raw deferred, so we ask + # ObservableDeferred to swallow the error. This is fine as the error will + # still be reported to the observers. + observable = ObservableDeferred(deferred, consumeErrors=True) + self.transactions[txn_key] = (observable, self.clock.time_msec()) return observable.observe() def _cleanup(self):