-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
http txns: Do not cache error responses #1913
Conversation
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
Outdated
# (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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not all errors have a .code
. Usually we do this sort of thing by adding an errBack
to the deferred to remove the entry from the cache
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should I do to check if there is a code
attribute? Try to cast to CodeMessageException
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.feb15dc
synapse/rest/client/transactions.py
Outdated
# 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] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd actually do this as self.transactions.pop(txn_key, None)
just in case the txn has already been deleted somehow. (del
will throw if it doesn't exsit)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.808ddf0
@erikjohnston PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Changes in synapse v0.19.3-rc1 (2017-03-08) =========================================== Features: * Add some administration functionalities. Thanks to morteza-araby! (PR #1784) Changes: * Reduce database table sizes (PR #1873, #1916, #1923, #1963) * Update contrib/ to not use syutil. Thanks to andrewshadura! (PR #1907) * Don't fetch current state when sending an event in common case (PR #1955) Bug fixes: * Fix synapse_port_db failure. Thanks to Pneumaticat! (PR #1904) * Fix caching to not cache error responses (PR #1913) * Fix APIs to make kick & ban reasons work (PR #1917) * Fix bugs in the /keys/changes api (PR #1921) * Fix bug where users couldn't forget rooms they were banned from (PR #1922) * Fix issue with long language values in pushers API (PR #1925) * Fix a race in transaction queue (PR #1930) * Fix dynamic thumbnailing to preserve aspect ratio. Thanks to jkolo! (PR #1945) * Fix device list update to not constantly resync (PR #1964) * Fix potential for huge memory usage when getting device that have changed (PR #1969)
Previously we did. This meant that, amongst other errors, rate-limiting errors
would be cached and prevent messages with that txn ID being sent.