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

Harden ResponseCache (and fix #9507) #9522

Closed
wants to merge 5 commits into from

Conversation

ShadowJonathan
Copy link
Contributor

@ShadowJonathan ShadowJonathan commented Mar 1, 2021

This fixes #9507 by making ResponseCache additionally not cache the result when;

  • The original function throws an exception.
  • One of the conditional callables, upon executing, throws an exception.

Previously, the latter could cause the cache not to be evicted inside the errback of the ObservableDeferred, because these conditionals would be evaluated (and have an exception thrown) before a callLater or immidiate cache eviction could be done, this what caused #9507 (most likely).

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
  • Pull request includes a sign off
  • Code style is correct (run the linters)

Signed-off-by: Jonathan de Jong <[email protected]>

Fixes #9507

@ShadowJonathan
Copy link
Contributor Author

(I also split the functions into class-level private methods, because it was getting too crowded with 3 local functions)

@ShadowJonathan ShadowJonathan mentioned this pull request Mar 2, 2021
4 tasks
Comment on lines +109 to +112
should_cache = all(
self._safe_call(func, r, key=key)
for func in self.pending_conditionals.pop(key, [])
)
Copy link
Member

Choose a reason for hiding this comment

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

for links: I've argued in #synapse-dev and https://github.com/matrix-org/synapse/pull/9358/files#r585481702 that we should only support one conditional per key, which simplifies this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add that to the "small changes to ResponseCache PR", this one is focused on fixing #9507, i dont wanna drag everything else into here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created #9525 for this

@richvdh richvdh requested a review from a team March 2, 2021 12:31
changelog.d/9522.bugfix Outdated Show resolved Hide resolved
@ShadowJonathan ShadowJonathan marked this pull request as draft March 2, 2021 15:01
@ShadowJonathan
Copy link
Contributor Author

Folded into #9739

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Synapse potentially cached a failed sync request
3 participants