Introduce SyncResponseCache and add /sync response cache timeout by ShadowJonathan · Pull Request #9739 · matrix-org/synapse

ShadowJonathan · 2021-04-03T18:27:41Z

This is a compromise between #9358's initial handling and the subsequent discussion for semantics (continuing in #9525).

This PR introduces a subclass to synapse.util.caches.response_cache.ResponseCache that adds the wrap_conditional behaviour that is applicable for /sync; the Very First Caller to the wrap* function gets to decide the condition-check function for it's result, whether or not it'll instantly expire.

This PR also adds an experimental config key: sync_cache_timeout_ms, the new cache timeout (in ms) for /sync results to expire (based on their initial arguments).

A few notes

I'll restate it; This PR is a compromise, I would want to continue the discussion in #9525 first, but I don't want to have that bog the fix for #8518, thus this PR adds an explicit subclass to signal that.

I will not merge SyncResponseCache with ResponseCache because of that, because I would want to have the discussion in #9525 first.

SyncResponseCache.wrap_conditional's behaviour is custom to /sync.

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)

Fixes #8518
Fixes #3880

Signed-off-by: Jonathan de Jong <jonathan@automatia.nl>

ShadowJonathan · 2021-04-03T18:33:54Z

(this is a bit over the 200-LoC limit, but 135 lines of these come from test_sync_responsecache.py, which is mostly boilerplate anyway.)

ShadowJonathan · 2021-04-03T18:47:15Z

changelog.d/9739.feature

@@ -0,0 +1 @@
+Added experimental support to cache `/sync` responses with config key `experimental_features.sync_cache_timeout_ms` (in milliseconds).


(Do we announce experimental settings like these? Is this even experimental?)

Well yes, we announce experimental features. I don't think this should be one though. Experimental features are more appropriate when we are making changes to the Matrix protocol; this is all internal to Synapse.

If it needs shaking out before releasing (and I'm not convinced it does), we can do that with test deployments using branches rather than with feature flags.

ShadowJonathan · 2021-04-03T18:47:50Z

synapse/util/caches/sync_response_cache.py

+                # (and log it)
+                res = cond(result)  # type: Any
+            except Exception:
+                logger.exception(


.exception because then it throws it to sentry, i think this is pretty sentry-worthy

ShadowJonathan · 2021-04-03T18:48:59Z

synapse/util/caches/sync_response_cache.py

+                    )
+                    # Return concrete boolean value based on falsy or truthiness.
+                    # If this raises, then so be it, then this value wasn't ever supposed to be true" or "false"
+                    # anyways, then have it be a scream test.


I could be more correct by wrapping a try-catch around this, but i think that'd be a bit too over-cautious, i dont know what situations could cause bool() to raise, but coupled with the limited use, i think this is fine.

Still added a comment, though.

(Yes, i'd like it to be this "correct")

ShadowJonathan · 2021-04-03T18:51:56Z

synapse/util/caches/sync_response_cache.py

+                    return res
+
+    # Copy this method wholesale from ResponseCache to be able to alter the inner `remove` function
+    def set(self, key: T, deferred: defer.Deferred) -> defer.Deferred:


I couldn't not copy this without editing ResponseCache to use a method instead of the ::{{remove}} closure, I didn't wanna touch ResponseCache, so i just copied this part.

Should I make ResponseCache.set more generic to allow for it's "remove" closure to be class-defined, or is this okay? (Could add a little bit more maintenance burden to leave it like this)

ShadowJonathan · 2021-04-03T18:52:42Z

tests/util/caches/test_sync_responsecache.py

+CALLBACK = lambda: OBJ
+
+# An object, can be equalized to itself
+OBJ = {0}


All that matters is that i have something with an identifier that x == x, so i can easily boilerplate it.

richvdh

well, I hear your point that you don't want to merge this with ResponseCache, but forking the code (even via a subclass) is a poor solution here: it means we have twice as much stuff to maintain, and having two similar-but-different things is always a recipe for confusion. (Haven't you complained about the two HttpClient implementations in the past?)

I don't really think this is the right solution, and I'm not inclined to accept it. The best I can suggest is waiting for now, and I'll try to put up a PR with my suggested approach and we can try to agree as a group which is the right way forward.

richvdh · 2021-04-08T16:25:41Z

changelog.d/9739.bugfix

@@ -0,0 +1 @@
+Fix #8518 and #3880.


These won't get auto-linked by towncrier. Please spell out the bugs you are fixing - linking back to the GH issue number isn't normally necessary because anyone who is really interested can follow the link to the PR and thence to the GH issues.

richvdh · 2021-04-08T16:25:42Z

changelog.d/9739.feature

@@ -0,0 +1 @@
+Added experimental support to cache `/sync` responses with config key `experimental_features.sync_cache_timeout_ms` (in milliseconds).


Well yes, we announce experimental features. I don't think this should be one though. Experimental features are more appropriate when we are making changes to the Matrix protocol; this is all internal to Synapse.

If it needs shaking out before releasing (and I'm not convinced it does), we can do that with test deployments using branches rather than with feature flags.

richvdh · 2021-06-09T23:38:10Z

I've finally put up my counter-proposal at #10157.

ShadowJonathan · 2021-06-10T14:59:12Z

Thanks, i'll close this PR so that all attention can be focused on that one.

ShadowJonathan added 3 commits April 3, 2021 20:20

add initial work

8ab3310

news

9a36ecb

more news

b965800

This was referenced Apr 3, 2021

Add option for timed /sync cache #9484

Closed

[discussion] Changing the behaviour of ResponseCache.wrap_conditional to only honour the very first caller #9525

Closed

Harden ResponseCache (and fix #9507) #9522

Closed

spacing

e8b487b

ShadowJonathan commented Apr 3, 2021

View reviewed changes

clokep requested a review from a team April 5, 2021 11:54

richvdh reviewed Apr 8, 2021

View reviewed changes

richvdh mentioned this pull request Jun 9, 2021

Extend ResponseCache to pass a context object into the callback #10157

Merged

ShadowJonathan closed this Jun 10, 2021

		@@ -0,0 +1 @@
		Added experimental support to cache `/sync` responses with config key `experimental_features.sync_cache_timeout_ms` (in milliseconds). No newline at end of file

Uh oh!

Conversation

ShadowJonathan commented Apr 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

A few notes

Pull Request Checklist

Uh oh!

ShadowJonathan commented Apr 3, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richvdh commented Jun 9, 2021

Uh oh!

ShadowJonathan commented Jun 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ShadowJonathan commented Apr 3, 2021 •

edited

Loading