From 8ecaff51a147948f977e745bace697ffcba6595b Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Fri, 11 Nov 2016 17:47:03 +0000 Subject: Review comments --- synapse/rest/client/transactions.py | 85 +++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) create mode 100644 synapse/rest/client/transactions.py (limited to 'synapse/rest/client/transactions.py') diff --git a/synapse/rest/client/transactions.py b/synapse/rest/client/transactions.py new file mode 100644 index 0000000000..1db972a378 --- /dev/null +++ b/synapse/rest/client/transactions.py @@ -0,0 +1,85 @@ +# -*- coding: utf-8 -*- +# Copyright 2014-2016 OpenMarket 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. + +"""This module contains logic for storing HTTP PUT transactions. This is used +to ensure idempotency when performing PUTs using the REST API.""" +import logging + +from synapse.api.auth import get_access_token_from_request +from synapse.util.async import ObservableDeferred + +logger = logging.getLogger(__name__) + + +def get_transaction_key(request): + """A helper function which returns a transaction key that can be used + with TransactionCache for idempotent requests. + + Idempotency is based on the returned key being the same for separate + requests to the same endpoint. The key is formed from the HTTP request + path and the access_token for the requesting user. + + Args: + request (twisted.web.http.Request): The incoming request. Must + contain an access_token. + Returns: + str: A transaction key + """ + token = get_access_token_from_request(request) + return request.path + "/" + token + + +class HttpTransactionCache(object): + + def __init__(self): + self.transactions = { + # $txn_key: ObservableDeferred<(res_code, res_json_body)> + } + + def fetch_or_execute_request(self, request, fn, *args, **kwargs): + """A helper function for fetch_or_execute which extracts + a transaction key from the given request. + + See: + fetch_or_execute + """ + return self.fetch_or_execute( + get_transaction_key(request), fn, *args, **kwargs + ) + + def fetch_or_execute(self, txn_key, fn, *args, **kwargs): + """Fetches the response for this transaction, or executes the given function + to produce a response for this transaction. + + Args: + txn_key (str): A key to ensure idempotency should fetch_or_execute be + called again at a later point in time. + fn (function): A function which returns a tuple of + (response_code, response_dict)d + *args: Arguments to pass to fn. + **kwargs: Keyword arguments to pass to fn. + Returns: + synapse.util.async.ObservableDeferred which resolves to a tuple + of (response_code, response_dict). + """ + try: + return self.transactions[txn_key] + except KeyError: + pass # execute the function instead. + + deferred = fn(*args, **kwargs) + observable = ObservableDeferred(deferred) + self.transactions[txn_key] = observable + return observable -- cgit 1.4.1 From af4a1bac5088e8083f55eea05f6fad44208a3a51 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Mon, 14 Nov 2016 09:52:41 +0000 Subject: Move .observe() up to the cache to make things neater --- synapse/rest/client/transactions.py | 9 ++++----- synapse/rest/client/v1/room.py | 30 ++++++---------------------- synapse/rest/client/v2_alpha/sendtodevice.py | 5 +---- 3 files changed, 11 insertions(+), 33 deletions(-) (limited to 'synapse/rest/client/transactions.py') diff --git a/synapse/rest/client/transactions.py b/synapse/rest/client/transactions.py index 1db972a378..8d69e12d36 100644 --- a/synapse/rest/client/transactions.py +++ b/synapse/rest/client/transactions.py @@ -67,19 +67,18 @@ class HttpTransactionCache(object): txn_key (str): A key to ensure idempotency should fetch_or_execute be called again at a later point in time. fn (function): A function which returns a tuple of - (response_code, response_dict)d + (response_code, response_dict). *args: Arguments to pass to fn. **kwargs: Keyword arguments to pass to fn. Returns: - synapse.util.async.ObservableDeferred which resolves to a tuple - of (response_code, response_dict). + Deferred which resolves to a tuple of (response_code, response_dict). """ try: - return self.transactions[txn_key] + return self.transactions[txn_key].observe() except KeyError: pass # execute the function instead. deferred = fn(*args, **kwargs) observable = ObservableDeferred(deferred) self.transactions[txn_key] = observable - return observable + return observable.observe() diff --git a/synapse/rest/client/v1/room.py b/synapse/rest/client/v1/room.py index 0622e64380..3fb1f2deb3 100644 --- a/synapse/rest/client/v1/room.py +++ b/synapse/rest/client/v1/room.py @@ -53,13 +53,10 @@ class RoomCreateRestServlet(ClientV1RestServlet): client_path_patterns("/createRoom(?:/.*)?$"), self.on_OPTIONS) - @defer.inlineCallbacks def on_PUT(self, request, txn_id): - observable = self.txns.fetch_or_execute_request( + return self.txns.fetch_or_execute_request( request, self.on_POST, request ) - res = yield observable.observe() - defer.returnValue(res) @defer.inlineCallbacks def on_POST(self, request): @@ -208,13 +205,10 @@ class RoomSendEventRestServlet(ClientV1RestServlet): def on_GET(self, request, room_id, event_type, txn_id): return (200, "Not implemented") - @defer.inlineCallbacks def on_PUT(self, request, room_id, event_type, txn_id): - observable = self.txns.fetch_or_execute_request( + return self.txns.fetch_or_execute_request( request, self.on_POST, request, room_id, event_type, txn_id ) - res = yield observable.observe() - defer.returnValue(res) # TODO: Needs unit testing for room ID + alias joins @@ -271,13 +265,10 @@ class JoinRoomAliasServlet(ClientV1RestServlet): defer.returnValue((200, {"room_id": room_id})) - @defer.inlineCallbacks def on_PUT(self, request, room_identifier, txn_id): - observable = self.txns.fetch_or_execute_request( + return self.txns.fetch_or_execute_request( request, self.on_POST, request, room_identifier, txn_id ) - res = yield observable.observe() - defer.returnValue(res) # TODO: Needs unit testing @@ -519,13 +510,10 @@ class RoomForgetRestServlet(ClientV1RestServlet): defer.returnValue((200, {})) - @defer.inlineCallbacks def on_PUT(self, request, room_id, txn_id): - observable = self.txns.fetch_or_execute_request( + return self.txns.fetch_or_execute_request( request, self.on_POST, request, room_id, txn_id ) - res = yield observable.observe() - defer.returnValue(res) # TODO: Needs unit testing @@ -597,13 +585,10 @@ class RoomMembershipRestServlet(ClientV1RestServlet): return False return True - @defer.inlineCallbacks def on_PUT(self, request, room_id, membership_action, txn_id): - observable = self.txns.fetch_or_execute_request( + return self.txns.fetch_or_execute_request( request, self.on_POST, request, room_id, membership_action, txn_id ) - res = yield observable.observe() - defer.returnValue(res) class RoomRedactEventRestServlet(ClientV1RestServlet): @@ -635,13 +620,10 @@ class RoomRedactEventRestServlet(ClientV1RestServlet): defer.returnValue((200, {"event_id": event.event_id})) - @defer.inlineCallbacks def on_PUT(self, request, room_id, event_id, txn_id): - observable = self.txns.fetch_or_execute_request( + return self.txns.fetch_or_execute_request( request, self.on_POST, request, room_id, event_id, txn_id ) - res = yield observable.observe() - defer.returnValue(res) class RoomTypingRestServlet(ClientV1RestServlet): diff --git a/synapse/rest/client/v2_alpha/sendtodevice.py b/synapse/rest/client/v2_alpha/sendtodevice.py index 2ce038c6cd..2187350d42 100644 --- a/synapse/rest/client/v2_alpha/sendtodevice.py +++ b/synapse/rest/client/v2_alpha/sendtodevice.py @@ -43,13 +43,10 @@ class SendToDeviceRestServlet(servlet.RestServlet): self.txns = HttpTransactionCache() self.device_message_handler = hs.get_device_message_handler() - @defer.inlineCallbacks def on_PUT(self, request, message_type, txn_id): - observable = self.txns.fetch_or_execute_request( + return self.txns.fetch_or_execute_request( request, self._put, request, message_type, txn_id ) - res = yield observable.observe() - defer.returnValue(res) @defer.inlineCallbacks def _put(self, request, message_type, txn_id): -- cgit 1.4.1 From 3991b4cbdb5f5fbdf61ad6efa879b3881143c214 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Mon, 14 Nov 2016 11:19:24 +0000 Subject: Clean transactions based on time. Add HttpTransactionCache tests. --- synapse/rest/client/transactions.py | 24 ++++++++-- synapse/rest/client/v1/base.py | 2 +- synapse/rest/client/v2_alpha/sendtodevice.py | 2 +- synapse/util/__init__.py | 10 +++- tests/rest/client/test_transactions.py | 69 ++++++++++++++++++++++++++++ 5 files changed, 99 insertions(+), 8 deletions(-) create mode 100644 tests/rest/client/test_transactions.py (limited to 'synapse/rest/client/transactions.py') diff --git a/synapse/rest/client/transactions.py b/synapse/rest/client/transactions.py index 8d69e12d36..351170edbc 100644 --- a/synapse/rest/client/transactions.py +++ b/synapse/rest/client/transactions.py @@ -41,12 +41,19 @@ def get_transaction_key(request): return request.path + "/" + token +CLEANUP_PERIOD_MS = 1000 * 60 * 30 # 30 mins + + class HttpTransactionCache(object): - def __init__(self): + def __init__(self, clock): + self.clock = clock self.transactions = { - # $txn_key: ObservableDeferred<(res_code, res_json_body)> + # $txn_key: (ObservableDeferred<(res_code, res_json_body)>, timestamp) } + # Try to clean entries every 30 mins. This means entries will exist + # for at *LEAST* 30 mins, and at *MOST* 60 mins. + self.cleaner = self.clock.looping_call(self._cleanup, CLEANUP_PERIOD_MS) def fetch_or_execute_request(self, request, fn, *args, **kwargs): """A helper function for fetch_or_execute which extracts @@ -74,11 +81,18 @@ class HttpTransactionCache(object): Deferred which resolves to a tuple of (response_code, response_dict). """ try: - return self.transactions[txn_key].observe() - except KeyError: + return self.transactions[txn_key][0].observe() + except (KeyError, IndexError): pass # execute the function instead. deferred = fn(*args, **kwargs) observable = ObservableDeferred(deferred) - self.transactions[txn_key] = observable + self.transactions[txn_key] = (observable, self.clock.time_msec()) return observable.observe() + + def _cleanup(self): + now = self.clock.time_msec() + for key in self.transactions.keys(): + ts = self.transactions[key][1] + if now > (ts + CLEANUP_PERIOD_MS): # after cleanup period + del self.transactions[key] diff --git a/synapse/rest/client/v1/base.py b/synapse/rest/client/v1/base.py index 07ff5b218c..c7aa0bbf59 100644 --- a/synapse/rest/client/v1/base.py +++ b/synapse/rest/client/v1/base.py @@ -60,4 +60,4 @@ class ClientV1RestServlet(RestServlet): self.hs = hs self.builder_factory = hs.get_event_builder_factory() self.auth = hs.get_v1auth() - self.txns = HttpTransactionCache() + self.txns = HttpTransactionCache(hs.get_clock()) diff --git a/synapse/rest/client/v2_alpha/sendtodevice.py b/synapse/rest/client/v2_alpha/sendtodevice.py index 2187350d42..ac660669f3 100644 --- a/synapse/rest/client/v2_alpha/sendtodevice.py +++ b/synapse/rest/client/v2_alpha/sendtodevice.py @@ -40,7 +40,7 @@ class SendToDeviceRestServlet(servlet.RestServlet): super(SendToDeviceRestServlet, self).__init__() self.hs = hs self.auth = hs.get_auth() - self.txns = HttpTransactionCache() + self.txns = HttpTransactionCache(hs.get_clock()) self.device_message_handler = hs.get_device_message_handler() def on_PUT(self, request, message_type, txn_id): diff --git a/synapse/util/__init__.py b/synapse/util/__init__.py index 2b3f0bef3c..c05b9450be 100644 --- a/synapse/util/__init__.py +++ b/synapse/util/__init__.py @@ -34,7 +34,7 @@ class Clock(object): """A small utility that obtains current time-of-day so that time may be mocked during unit-tests. - TODO(paul): Also move the sleep() functionallity into it + TODO(paul): Also move the sleep() functionality into it """ def time(self): @@ -46,6 +46,14 @@ class Clock(object): return int(self.time() * 1000) def looping_call(self, f, msec): + """Call a function repeatedly. + + Waits `msec` initially before calling `f` for the first time. + + Args: + f(function): The function to call repeatedly. + msec(float): How long to wait between calls in milliseconds. + """ l = task.LoopingCall(f) l.start(msec / 1000.0, now=False) return l diff --git a/tests/rest/client/test_transactions.py b/tests/rest/client/test_transactions.py new file mode 100644 index 0000000000..d7cea30260 --- /dev/null +++ b/tests/rest/client/test_transactions.py @@ -0,0 +1,69 @@ +from synapse.rest.client.transactions import HttpTransactionCache +from synapse.rest.client.transactions import CLEANUP_PERIOD_MS +from twisted.internet import defer +from mock import Mock, call +from tests import unittest +from tests.utils import MockClock + + +class HttpTransactionCacheTestCase(unittest.TestCase): + + def setUp(self): + self.clock = MockClock() + self.cache = HttpTransactionCache(self.clock) + + self.mock_http_response = (200, "GOOD JOB!") + self.mock_key = "foo" + + @defer.inlineCallbacks + def test_executes_given_function(self): + cb = Mock( + return_value=defer.succeed(self.mock_http_response) + ) + res = yield self.cache.fetch_or_execute( + self.mock_key, cb, "some_arg", keyword="arg" + ) + cb.assert_called_once_with("some_arg", keyword="arg") + self.assertEqual(res, self.mock_http_response) + + @defer.inlineCallbacks + def test_deduplicates_based_on_key(self): + cb = Mock( + return_value=defer.succeed(self.mock_http_response) + ) + for i in range(3): # invoke multiple times + res = yield self.cache.fetch_or_execute( + self.mock_key, cb, "some_arg", keyword="arg", changing_args=i + ) + self.assertEqual(res, self.mock_http_response) + # expect only a single call to do the work + cb.assert_called_once_with("some_arg", keyword="arg", changing_args=0) + + @defer.inlineCallbacks + def test_cleans_up(self): + cb = Mock( + return_value=defer.succeed(self.mock_http_response) + ) + yield self.cache.fetch_or_execute( + self.mock_key, cb, "an arg" + ) + # should NOT have cleaned up yet + self.clock.advance_time_msec(CLEANUP_PERIOD_MS / 2) + + yield self.cache.fetch_or_execute( + self.mock_key, cb, "an arg" + ) + # still using cache + cb.assert_called_once_with("an arg") + + self.clock.advance_time_msec(CLEANUP_PERIOD_MS) + + yield self.cache.fetch_or_execute( + self.mock_key, cb, "an arg" + ) + # no longer using cache + self.assertEqual(cb.call_count, 2) + self.assertEqual( + cb.call_args_list, + [call("an arg",), call("an arg",)] + ) -- cgit 1.4.1