Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

http txns: Do not cache error responses #1913

Merged
merged 4 commits into from
Feb 13, 2017
Merged

Conversation

kegsay
Copy link
Member

@kegsay kegsay commented Feb 13, 2017

Previously we did. This meant that, amongst other errors, rate-limiting errors
would be cached and prevent messages with that txn ID being sent.

Previously we did. This meant that, amongst other errors, rate-limiting errors
would be cached and prevent messages with that txn ID being sent.
@kegsay kegsay added the z-bug (Deprecated Label) label Feb 13, 2017
# (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:
Copy link
Member

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

Copy link
Member Author

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.feb15dc

# 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]
Copy link
Member

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.808ddf0

@kegsay
Copy link
Member Author

kegsay commented Feb 13, 2017

@erikjohnston PTAL

Copy link
Member

@erikjohnston erikjohnston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@erikjohnston erikjohnston merged commit 359c97f into develop Feb 13, 2017
@kegsay kegsay mentioned this pull request Feb 13, 2017
erikjohnston added a commit that referenced this pull request Mar 13, 2017
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)
@hawkowl hawkowl deleted the kegan/dont-cache-errors branch September 20, 2018 14:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
z-bug (Deprecated Label)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants