From 2771447c29402e1f26bb45bdb730bdd0fe02682f Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Thu, 10 Nov 2016 14:49:26 +0000 Subject: Store Promise instead of Response for HTTP API transactions This fixes a race whereby: - User hits an endpoint. - No cached transaction so executes main code. - User hits same endpoint. - No cache transaction so executes main code. - Main code finishes executing and caches response and returns. - Main code finishes executing and caches response and returns. This race is common in the wild when Synapse is struggling under load. This commit fixes the race by: - User hits an endpoint. - Caches the promise to execute the main code and executes main code. - User hits same endpoint. - Yields on the same promise as the first request. - Main code finishes executing and returns, unblocking both requests. --- synapse/rest/client/v1/transactions.py | 52 ++++++++++------------------------ 1 file changed, 15 insertions(+), 37 deletions(-) (limited to 'synapse/rest/client/v1/transactions.py') diff --git a/synapse/rest/client/v1/transactions.py b/synapse/rest/client/v1/transactions.py index 2f2c9d0881..f5012c5f59 100644 --- a/synapse/rest/client/v1/transactions.py +++ b/synapse/rest/client/v1/transactions.py @@ -22,57 +22,35 @@ from synapse.api.auth import get_access_token_from_request logger = logging.getLogger(__name__) -# FIXME: elsewhere we use FooStore to indicate something in the storage layer... -class HttpTransactionStore(object): +class HttpTransactionCache(object): def __init__(self): - # { key : (txn_id, response) } + # { key : (txn_id, response_deferred) } self.transactions = {} - def get_response(self, key, txn_id): - """Retrieve a response for this request. - - Args: - key (str): A transaction-independent key for this request. Usually - this is a combination of the path (without the transaction id) - and the user's access token. - txn_id (str): The transaction ID for this request - Returns: - A tuple of (HTTP response code, response content) or None. - """ + def _get_response(self, key, txn_id): try: - logger.debug("get_response TxnId: %s", txn_id) - (last_txn_id, response) = self.transactions[key] + (last_txn_id, response_deferred) = self.transactions[key] if txn_id == last_txn_id: logger.info("get_response: Returning a response for %s", txn_id) - return response + return response_deferred except KeyError: pass return None - def store_response(self, key, txn_id, response): - """Stores an HTTP response tuple. - - Args: - key (str): A transaction-independent key for this request. Usually - this is a combination of the path (without the transaction id) - and the user's access token. - txn_id (str): The transaction ID for this request. - response (tuple): A tuple of (HTTP response code, response content) - """ - logger.debug("store_response TxnId: %s", txn_id) - self.transactions[key] = (txn_id, response) + def _store_response(self, key, txn_id, response_deferred): + self.transactions[key] = (txn_id, response_deferred) - def store_client_transaction(self, request, txn_id, response): - """Stores the request/response pair of an HTTP transaction. + def store_client_transaction(self, request, txn_id, response_deferred): + """Stores the request/Promise pair of an HTTP transaction. Args: request (twisted.web.http.Request): The twisted HTTP request. This request must have the transaction ID as the last path segment. - response (tuple): A tuple of (response code, response dict) + response_deferred (Promise): A tuple of (response code, response dict) txn_id (str): The transaction ID for this request. """ - self.store_response(self._get_key(request), txn_id, response) + self._store_response(self._get_key(request), txn_id, response_deferred) def get_client_transaction(self, request, txn_id): """Retrieves a stored response if there was one. @@ -82,14 +60,14 @@ class HttpTransactionStore(object): request must have the transaction ID as the last path segment. txn_id (str): The transaction ID for this request. Returns: - The response tuple. + Promise: Resolves to the response tuple. Raises: KeyError if the transaction was not found. """ - response = self.get_response(self._get_key(request), txn_id) - if response is None: + response_deferred = self._get_response(self._get_key(request), txn_id) + if response_deferred is None: raise KeyError("Transaction not found.") - return response + return response_deferred def _get_key(self, request): token = get_access_token_from_request(request) -- cgit 1.5.1 From 42c43cfafd3e0471e4e0f6fb05e951290783ba1f Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Fri, 11 Nov 2016 14:54:10 +0000 Subject: Use ObservableDeferreds instead of Deferreds as they behave as intended --- synapse/rest/client/v1/room.py | 39 +++++++++++++++++----------------- synapse/rest/client/v1/transactions.py | 22 +++++++++---------- 2 files changed, 31 insertions(+), 30 deletions(-) (limited to 'synapse/rest/client/v1/transactions.py') diff --git a/synapse/rest/client/v1/room.py b/synapse/rest/client/v1/room.py index caa779ffaf..47c4b84b73 100644 --- a/synapse/rest/client/v1/room.py +++ b/synapse/rest/client/v1/room.py @@ -22,6 +22,7 @@ from synapse.streams.config import PaginationConfig from synapse.api.constants import EventTypes, Membership from synapse.api.filtering import Filter from synapse.types import UserID, RoomID, RoomAlias +from synapse.util.async import ObservableDeferred from synapse.events.utils import serialize_event, format_event_for_client_v2 from synapse.http.servlet import ( parse_json_object_from_request, parse_string, parse_integer @@ -57,14 +58,14 @@ class RoomCreateRestServlet(ClientV1RestServlet): def on_PUT(self, request, txn_id): try: res_deferred = self.txns.get_client_transaction(request, txn_id) - res = yield res_deferred + res = yield res_deferred.observe() defer.returnValue(res) except KeyError: pass - res_deferred = self.on_POST(request) + res_deferred = ObservableDeferred(self.on_POST(request)) self.txns.store_client_transaction(request, txn_id, res_deferred) - response = yield res_deferred + response = yield res_deferred.observe() defer.returnValue(response) @defer.inlineCallbacks @@ -218,14 +219,14 @@ class RoomSendEventRestServlet(ClientV1RestServlet): def on_PUT(self, request, room_id, event_type, txn_id): try: res_deferred = self.txns.get_client_transaction(request, txn_id) - res = yield res_deferred + res = yield res_deferred.observe() defer.returnValue(res) except KeyError: pass - res_deferred = self.on_POST(request, room_id, event_type, txn_id) + res_deferred = ObservableDeferred(self.on_POST(request, room_id, event_type, txn_id)) self.txns.store_client_transaction(request, txn_id, res_deferred) - response = yield res_deferred + response = yield res_deferred.observe() defer.returnValue(response) @@ -287,14 +288,14 @@ class JoinRoomAliasServlet(ClientV1RestServlet): def on_PUT(self, request, room_identifier, txn_id): try: res_deferred = self.txns.get_client_transaction(request, txn_id) - res = yield res_deferred + res = yield res_deferred.observe() defer.returnValue(res) except KeyError: pass - res_deferred = self.on_POST(request, room_identifier, txn_id) + res_deferred = ObservableDeferred(self.on_POST(request, room_identifier, txn_id)) self.txns.store_client_transaction(request, txn_id, res_deferred) - response = yield res_deferred + response = yield res_deferred.observe() defer.returnValue(response) @@ -541,14 +542,14 @@ class RoomForgetRestServlet(ClientV1RestServlet): def on_PUT(self, request, room_id, txn_id): try: res_deferred = self.txns.get_client_transaction(request, txn_id) - res = yield res_deferred + res = yield res_deferred.observe() defer.returnValue(res) except KeyError: pass - res_deferred = self.on_POST(request, room_id, txn_id) + res_deferred = ObservableDeferred(self.on_POST(request, room_id, txn_id)) self.txns.store_client_transaction(request, txn_id, res_deferred) - response = yield res_deferred + response = yield res_deferred.observe() defer.returnValue(response) @@ -624,15 +625,15 @@ class RoomMembershipRestServlet(ClientV1RestServlet): @defer.inlineCallbacks def on_PUT(self, request, room_id, membership_action, txn_id): try: - res_deferred = self.txns.get_client_transaction(request, txn_id) - res = yield res_deferred + res_deferred = ObservableDeferred(self.txns.get_client_transaction(request, txn_id)) + res = yield res_deferred.observe() defer.returnValue(res) except KeyError: pass - res_deferred = self.on_POST(request, room_id, membership_action, txn_id) + res_deferred = ObservableDeffself.on_POST(request, room_id, membership_action, txn_id) self.txns.store_client_transaction(request, txn_id, res_deferred) - response = yield res_deferred + response = yield res_deferred.observe() defer.returnValue(response) @@ -669,14 +670,14 @@ class RoomRedactEventRestServlet(ClientV1RestServlet): def on_PUT(self, request, room_id, event_id, txn_id): try: res_deferred = self.txns.get_client_transaction(request, txn_id) - res = yield res_deferred + res = yield res_deferred.observe() defer.returnValue(res) except KeyError: pass - res_deferred = self.on_POST(request, room_id, event_id, txn_id) + res_deferred = ObservableDeferred(self.on_POST(request, room_id, event_id, txn_id)) self.txns.store_client_transaction(request, txn_id, res_deferred) - response = yield res_deferred + response = yield res_deferred.observe() defer.returnValue(response) diff --git a/synapse/rest/client/v1/transactions.py b/synapse/rest/client/v1/transactions.py index f5012c5f59..774430458a 100644 --- a/synapse/rest/client/v1/transactions.py +++ b/synapse/rest/client/v1/transactions.py @@ -25,32 +25,32 @@ logger = logging.getLogger(__name__) class HttpTransactionCache(object): def __init__(self): - # { key : (txn_id, response_deferred) } + # { key : (txn_id, res_observ_defer) } self.transactions = {} def _get_response(self, key, txn_id): try: - (last_txn_id, response_deferred) = self.transactions[key] + (last_txn_id, res_observ_defer) = self.transactions[key] if txn_id == last_txn_id: logger.info("get_response: Returning a response for %s", txn_id) - return response_deferred + return res_observ_defer except KeyError: pass return None - def _store_response(self, key, txn_id, response_deferred): - self.transactions[key] = (txn_id, response_deferred) + def _store_response(self, key, txn_id, res_observ_defer): + self.transactions[key] = (txn_id, res_observ_defer) - def store_client_transaction(self, request, txn_id, response_deferred): + def store_client_transaction(self, request, txn_id, res_observ_defer): """Stores the request/Promise pair of an HTTP transaction. Args: request (twisted.web.http.Request): The twisted HTTP request. This request must have the transaction ID as the last path segment. - response_deferred (Promise): A tuple of (response code, response dict) + res_observ_defer (Promise): A tuple of (response code, response dict) txn_id (str): The transaction ID for this request. """ - self._store_response(self._get_key(request), txn_id, response_deferred) + self._store_response(self._get_key(request), txn_id, res_observ_defer) def get_client_transaction(self, request, txn_id): """Retrieves a stored response if there was one. @@ -64,10 +64,10 @@ class HttpTransactionCache(object): Raises: KeyError if the transaction was not found. """ - response_deferred = self._get_response(self._get_key(request), txn_id) - if response_deferred is None: + res_observ_defer = self._get_response(self._get_key(request), txn_id) + if res_observ_defer is None: raise KeyError("Transaction not found.") - return response_deferred + return res_observ_defer def _get_key(self, request): token = get_access_token_from_request(request) -- cgit 1.5.1 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 ++++++++++++++++++++++++ synapse/rest/client/v1/base.py | 3 +- synapse/rest/client/v1/room.py | 97 ++++++++-------------------- synapse/rest/client/v1/transactions.py | 75 --------------------- synapse/rest/client/v2_alpha/sendtodevice.py | 17 ++--- 5 files changed, 119 insertions(+), 158 deletions(-) create mode 100644 synapse/rest/client/transactions.py delete mode 100644 synapse/rest/client/v1/transactions.py (limited to 'synapse/rest/client/v1/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 diff --git a/synapse/rest/client/v1/base.py b/synapse/rest/client/v1/base.py index 22c740c30c..07ff5b218c 100644 --- a/synapse/rest/client/v1/base.py +++ b/synapse/rest/client/v1/base.py @@ -18,7 +18,8 @@ from synapse.http.servlet import RestServlet from synapse.api.urls import CLIENT_PREFIX -from .transactions import HttpTransactionCache +from synapse.rest.client.transactions import HttpTransactionCache + import re import logging diff --git a/synapse/rest/client/v1/room.py b/synapse/rest/client/v1/room.py index 7af4077721..0622e64380 100644 --- a/synapse/rest/client/v1/room.py +++ b/synapse/rest/client/v1/room.py @@ -22,7 +22,6 @@ from synapse.streams.config import PaginationConfig from synapse.api.constants import EventTypes, Membership from synapse.api.filtering import Filter from synapse.types import UserID, RoomID, RoomAlias -from synapse.util.async import ObservableDeferred from synapse.events.utils import serialize_event, format_event_for_client_v2 from synapse.http.servlet import ( parse_json_object_from_request, parse_string, parse_integer @@ -56,17 +55,11 @@ class RoomCreateRestServlet(ClientV1RestServlet): @defer.inlineCallbacks def on_PUT(self, request, txn_id): - try: - res_deferred = self.txns.get_client_transaction(request, txn_id) - res = yield res_deferred.observe() - defer.returnValue(res) - except KeyError: - pass - - res_deferred = ObservableDeferred(self.on_POST(request)) - self.txns.store_client_transaction(request, txn_id, res_deferred) - response = yield res_deferred.observe() - defer.returnValue(response) + observable = 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): @@ -217,19 +210,11 @@ class RoomSendEventRestServlet(ClientV1RestServlet): @defer.inlineCallbacks def on_PUT(self, request, room_id, event_type, txn_id): - try: - res_deferred = self.txns.get_client_transaction(request, txn_id) - res = yield res_deferred.observe() - defer.returnValue(res) - except KeyError: - pass - - res_deferred = ObservableDeferred( - self.on_POST(request, room_id, event_type, txn_id) + observable = self.txns.fetch_or_execute_request( + request, self.on_POST, request, room_id, event_type, txn_id ) - self.txns.store_client_transaction(request, txn_id, res_deferred) - response = yield res_deferred.observe() - defer.returnValue(response) + res = yield observable.observe() + defer.returnValue(res) # TODO: Needs unit testing for room ID + alias joins @@ -288,17 +273,11 @@ class JoinRoomAliasServlet(ClientV1RestServlet): @defer.inlineCallbacks def on_PUT(self, request, room_identifier, txn_id): - try: - res_deferred = self.txns.get_client_transaction(request, txn_id) - res = yield res_deferred.observe() - defer.returnValue(res) - except KeyError: - pass - - res_deferred = ObservableDeferred(self.on_POST(request, room_identifier, txn_id)) - self.txns.store_client_transaction(request, txn_id, res_deferred) - response = yield res_deferred.observe() - defer.returnValue(response) + observable = 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 @@ -542,17 +521,11 @@ class RoomForgetRestServlet(ClientV1RestServlet): @defer.inlineCallbacks def on_PUT(self, request, room_id, txn_id): - try: - res_deferred = self.txns.get_client_transaction(request, txn_id) - res = yield res_deferred.observe() - defer.returnValue(res) - except KeyError: - pass - - res_deferred = ObservableDeferred(self.on_POST(request, room_id, txn_id)) - self.txns.store_client_transaction(request, txn_id, res_deferred) - response = yield res_deferred.observe() - defer.returnValue(response) + observable = 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 @@ -626,19 +599,11 @@ class RoomMembershipRestServlet(ClientV1RestServlet): @defer.inlineCallbacks def on_PUT(self, request, room_id, membership_action, txn_id): - try: - res_deferred = self.txns.get_client_transaction(request, txn_id) - res = yield res_deferred.observe() - defer.returnValue(res) - except KeyError: - pass - - res_deferred = ObservableDeferred( - self.on_POST(request, room_id, membership_action, txn_id) + observable = self.txns.fetch_or_execute_request( + request, self.on_POST, request, room_id, membership_action, txn_id ) - self.txns.store_client_transaction(request, txn_id, res_deferred) - response = yield res_deferred.observe() - defer.returnValue(response) + res = yield observable.observe() + defer.returnValue(res) class RoomRedactEventRestServlet(ClientV1RestServlet): @@ -672,19 +637,11 @@ class RoomRedactEventRestServlet(ClientV1RestServlet): @defer.inlineCallbacks def on_PUT(self, request, room_id, event_id, txn_id): - try: - res_deferred = self.txns.get_client_transaction(request, txn_id) - res = yield res_deferred.observe() - defer.returnValue(res) - except KeyError: - pass - - res_deferred = ObservableDeferred( - self.on_POST(request, room_id, event_id, txn_id) + observable = self.txns.fetch_or_execute_request( + request, self.on_POST, request, room_id, event_id, txn_id ) - self.txns.store_client_transaction(request, txn_id, res_deferred) - response = yield res_deferred.observe() - defer.returnValue(response) + res = yield observable.observe() + defer.returnValue(res) class RoomTypingRestServlet(ClientV1RestServlet): diff --git a/synapse/rest/client/v1/transactions.py b/synapse/rest/client/v1/transactions.py deleted file mode 100644 index 774430458a..0000000000 --- a/synapse/rest/client/v1/transactions.py +++ /dev/null @@ -1,75 +0,0 @@ -# -*- 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 - -logger = logging.getLogger(__name__) - - -class HttpTransactionCache(object): - - def __init__(self): - # { key : (txn_id, res_observ_defer) } - self.transactions = {} - - def _get_response(self, key, txn_id): - try: - (last_txn_id, res_observ_defer) = self.transactions[key] - if txn_id == last_txn_id: - logger.info("get_response: Returning a response for %s", txn_id) - return res_observ_defer - except KeyError: - pass - return None - - def _store_response(self, key, txn_id, res_observ_defer): - self.transactions[key] = (txn_id, res_observ_defer) - - def store_client_transaction(self, request, txn_id, res_observ_defer): - """Stores the request/Promise pair of an HTTP transaction. - - Args: - request (twisted.web.http.Request): The twisted HTTP request. This - request must have the transaction ID as the last path segment. - res_observ_defer (Promise): A tuple of (response code, response dict) - txn_id (str): The transaction ID for this request. - """ - self._store_response(self._get_key(request), txn_id, res_observ_defer) - - def get_client_transaction(self, request, txn_id): - """Retrieves a stored response if there was one. - - Args: - request (twisted.web.http.Request): The twisted HTTP request. This - request must have the transaction ID as the last path segment. - txn_id (str): The transaction ID for this request. - Returns: - Promise: Resolves to the response tuple. - Raises: - KeyError if the transaction was not found. - """ - res_observ_defer = self._get_response(self._get_key(request), txn_id) - if res_observ_defer is None: - raise KeyError("Transaction not found.") - return res_observ_defer - - def _get_key(self, request): - token = get_access_token_from_request(request) - path_without_txn_id = request.path.rsplit("/", 1)[0] - return path_without_txn_id + "/" + token diff --git a/synapse/rest/client/v2_alpha/sendtodevice.py b/synapse/rest/client/v2_alpha/sendtodevice.py index 31167ba535..2ce038c6cd 100644 --- a/synapse/rest/client/v2_alpha/sendtodevice.py +++ b/synapse/rest/client/v2_alpha/sendtodevice.py @@ -19,8 +19,7 @@ from twisted.internet import defer from synapse.http import servlet from synapse.http.servlet import parse_json_object_from_request -from synapse.rest.client.v1.transactions import HttpTransactionCache -from synapse.util.async import ObservableDeferred +from synapse.rest.client.transactions import HttpTransactionCache from ._base import client_v2_patterns @@ -46,16 +45,10 @@ class SendToDeviceRestServlet(servlet.RestServlet): @defer.inlineCallbacks def on_PUT(self, request, message_type, txn_id): - try: - res_deferred = self.txns.get_client_transaction(request, txn_id) - res = yield res_deferred.observe() - defer.returnValue(res) - except KeyError: - pass - - res_deferred = ObservableDeferred(self._put(request, message_type, txn_id)) - self.txns.store_client_transaction(request, txn_id, res_deferred) - res = yield res_deferred.observe() + observable = self.txns.fetch_or_execute_request( + request, self._put, request, message_type, txn_id + ) + res = yield observable.observe() defer.returnValue(res) @defer.inlineCallbacks -- cgit 1.5.1