Merge pull request #3991 from matrix-org/erikj/fix_pop_retry_cache

Fix bug when invalidating destination retry timings

3 files changed, 47 insertions, 1 deletions

diff --git a/changelog.d/3991.bugfix b/changelog.d/3991.bugfix
new file mode 100644
index 0000000000..6545871f55
--- /dev/null
+++ b/changelog.d/3991.bugfix
@@ -0,0 +1 @@
+Add a cache to get_destination_retry_timings
diff --git a/synapse/storage/transactions.py b/synapse/storage/transactions.py
index ab54977a75..a3032cdce9 100644
--- a/synapse/storage/transactions.py
+++ b/synapse/storage/transactions.py
@@ -216,7 +216,7 @@ class TransactionStore(SQLBaseStore):
             retry_interval (int) - how long until next retry in ms
         """
 
-        self._destination_retry_cache.pop(destination)
+        self._destination_retry_cache.pop(destination, None)
         return self.runInteraction(
             "set_destination_retry_timings",
             self._set_destination_retry_timings,
diff --git a/tests/storage/test_transactions.py b/tests/storage/test_transactions.py
new file mode 100644
index 0000000000..14169afa96
--- /dev/null
+++ b/tests/storage/test_transactions.py
@@ -0,0 +1,45 @@
+# -*- coding: utf-8 -*-
+# Copyright 2018 New Vector Ltd
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+from tests.unittest import HomeserverTestCase
+
+
+class TransactionStoreTestCase(HomeserverTestCase):
+    def prepare(self, reactor, clock, homeserver):
+        self.store = homeserver.get_datastore()
+
+    def test_get_set_transactions(self):
+        """Tests that we can successfully get a non-existent entry for
+        destination retries, as well as testing tht we can set and get
+        correctly.
+        """
+        d = self.store.get_destination_retry_timings("example.com")
+        r = self.get_success(d)
+        self.assertIsNone(r)
+
+        d = self.store.set_destination_retry_timings("example.com", 50, 100)
+        self.get_success(d)
+
+        d = self.store.get_destination_retry_timings("example.com")
+        r = self.get_success(d)
+
+        self.assert_dict({"retry_last_ts": 50, "retry_interval": 100}, r)
+
+    def test_initial_set_transactions(self):
+        """Tests that we can successfully set the destination retries (there
+        was a bug around invalidating the cache that broke this)
+        """
+        d = self.store.set_destination_retry_timings("example.com", 50, 100)
+        self.get_success(d)