Performance improvements and refactor of Ratelimiter #7595

anoadragon453 · 2020-05-28T21:55:57Z

While working on #5665 I found myself digging into the Ratelimiter class and seeing that it was both:

Rather undocumented, and
causing a lot of config checks

This PR attempts to refactor and comment the Ratelimiter class, as well as encourage config file accesses to only be done at instantiation.

Best to be reviewed commit-by-commit.

This makes it simple for tests to modify/nullify them.

clokep

Overall this seems OK-ish, but I'm not convinced that moving the rate limiters to the homeserver object is an improvement, it seems to just move them farther from where they're used. Wouldn't we get the same benefit by instantiating them with the config values in the each handler's __init__? Is there another benefit I'm missing.

Instantiating the rate limiters with their config is much cleaner, but it is weird that we then touch internals of the rate limiters in one case, I think that needs a bit more of a think.

synapse/api/ratelimiting.py

tests/rest/client/v1/test_rooms.py

synapse/handlers/auth.py

synapse/handlers/_base.py

anoadragon453 · 2020-05-29T17:34:28Z

Overall this seems OK-ish, but I'm not convinced that moving the rate limiters to the homeserver object is an improvement, it seems to just move them farther from where they're used.

The benefit was supposed to be that even if it was used in multiple places in Synapse, the tests could hook in at a single point and modify it, however I now realize that:

By splitting the ratelimiters out, each one is only ever pulled from a single place in Synapse (sans the tests)
The tests should probably just modify the config file instead.

Edit: Ah, actually the registration ratelimiter is used in two different files. I could leave it in server or just put it in one and pull it from the other...

Edit 2:

The tests should probably just modify the config file instead.

Actually, it's a little dirty to set rate_hz and burst_count to 99999999 in the tests, so mocking the methods is probably still the cleanest way to go.

Other than the registration ratelimiter, as that's used in multiple classes and needs to keep the same state

anoadragon453 · 2020-06-01T18:39:11Z

Right, so in short, in this PR we:

Refactor Ratelimiter class to allow setting clock, default rate_hz and burst_count on object instantiation, while still keeping the ability to override per request. Nothing else should've changed here functionality-wise.
Move most of the instantiation of Ratelimiter into the classes that actually used them. The registration ratelimiter remains in HomeServer as it's used across multiple classes.
Change up the tests a bit to patch Ratelimiter in some areas instead of mocking it. This is easier than grabbing each individual ratelimiter and mocking its member functions, and the tests in those classes don't test for ratelimit stuff anywho.
For the tests that do care about ratelimiting, those tests have been modified to set config variables instead of directly modifying runtime objects.

This PR has the advantage of:

Making the Ratelimiter class easier to maintain.
Making ratelimiter calling code simpler.
Making less config calls.

old

anoadragon453 · 2020-06-01T18:40:40Z

@clokep Should hopefully be reviewable by commit from where you last left off. Excuse the cruft at the end. Let me know if I could break things down further in any way that would make review easier.

synapse/rest/client/v1/login.py

changelog.d/7595.misc

synapse/api/ratelimiting.py

tests/api/test_ratelimiting.py

anoadragon453 · 2020-06-03T16:00:55Z

synapse/api/ratelimiting.py

-            # This does not apply
+            # XXX: Why is this -1? This seems to only be used in
+            # self.ratelimit. I guess so that clients get a time in the past and don't
+            # feel afraid to try again immediately


Note: I'm tempted to resolve this, but I don't want to change the behaviour of Ratelimiter in this PR any more.

I agree that this sounds like a follow-up!

clokep · 2020-06-04T16:15:57Z

@anoadragon453 I pushed a couple of commits c145c81 and 12b4d47 that clean-up the tests a bit. Turns out we don't seem to need those ratelimiters defined there? Let me know if you think it looks wrong though.

clokep

I think this looks good! I asked for some minor tweaks to the tests to make them clearer.

synapse/api/ratelimiting.py

clokep · 2020-06-04T16:39:42Z

synapse/api/ratelimiting.py

-        self.message_counts = (
-            OrderedDict()
-        )  # type: OrderedDict[Any, Tuple[float, int, Optional[float]]]
+    def __init__(self, clock: Clock, rate_hz: float, burst_count: int):


I debated two things here:

Making rate_hz / burst_count optional so we don't have the case where we define them as 0 to override them each time.

Having two rate limiters (with one being a sub-class of the other) for the two situations (per key limit vs. per rate limiter limit).

I think these are both just bikeshedding at this point though and it isn't worth poking at more. I just wanted to write them down!

Neat ideas, will keep them in mind if we decide to do another refactor at some point. Thanks!

tests/api/test_ratelimiting.py

clokep · 2020-06-04T16:44:22Z

tests/api/test_ratelimiting.py

+        # We expect a LimitExceededError to be raised
+        try:
+            limiter.ratelimit(("test_id",), _time_now_s=time_now, update=False)
+
+            # We shouldn't reach here
+            self.assertTrue(False, "LimitExceededError was not raised")
+        except LimitExceededError as e:
+            self.assertEquals(e.retry_after_ms / 1000, expected_allowed - time_now)


This would be nicer using the context manager style:

with self.assertRaises(LimiteExceedError) as e: limiter.rateLimit(...) # Use e below self.assertEquals(...)

Was not aware this was supported by self.assertRaises. Thanks for the tip!

tests/api/test_ratelimiting.py

…napse into anoa/ratelimit_config_perf

clokep

I think this looks good. 🤞

@dklimpel

…rg-hotfixes Synapse 1.15.0rc1 (2020-06-09) ============================== Features -------- - Advertise support for Client-Server API r0.6.0 and remove related unstable feature flags. ([\#6585](#6585)) - Add an option to disable autojoining rooms for guest accounts. ([\#6637](#6637)) - For SAML authentication, add the ability to pass email addresses to be added to new users' accounts via SAML attributes. Contributed by Christopher Cooper. ([\#7385](#7385)) - Add admin APIs to allow server admins to manage users' devices. Contributed by @dklimpel. ([\#7481](#7481)) - Add support for generating thumbnails for WebP images. Previously, users would see an empty box instead of preview image. ([\#7586](#7586)) - Support the standardized `m.login.sso` user-interactive authentication flow. ([\#7630](#7630)) Bugfixes -------- - Allow new users to be registered via the admin API even if the monthly active user limit has been reached. Contributed by @dkimpel. ([\#7263](#7263)) - Fix email notifications not being enabled for new users when created via the Admin API. ([\#7267](#7267)) - Fix str placeholders in an instance of `PrepareDatabaseException`. Introduced in Synapse v1.8.0. ([\#7575](#7575)) - Fix a bug in automatic user creation during first time login with `m.login.jwt`. Regression in v1.6.0. Contributed by @olof. ([\#7585](#7585)) - Fix a bug causing the cross-signing keys to be ignored when resyncing a device list. ([\#7594](#7594)) - Fix metrics failing when there is a large number of active background processes. ([\#7597](#7597)) - Fix bug where returning rooms for a group would fail if it included a room that the server was not in. ([\#7599](#7599)) - Fix duplicate key violation when persisting read markers. ([\#7607](#7607)) - Prevent an entire iteration of the device list resync loop from failing if one server responds with a malformed result. ([\#7609](#7609)) - Fix exceptions when fetching events from a remote host fails. ([\#7622](#7622)) - Make `synctl restart` start synapse if it wasn't running. ([\#7624](#7624)) - Pass device information through to the login endpoint when using the login fallback. ([\#7629](#7629)) - Advertise the `m.login.token` login flow when OpenID Connect is enabled. ([\#7631](#7631)) - Fix bug in account data replication stream. ([\#7656](#7656)) Improved Documentation ---------------------- - Update the OpenBSD installation instructions. ([\#7587](#7587)) - Advertise Python 3.8 support in `setup.py`. ([\#7602](#7602)) - Add a link to `#synapse:matrix.org` in the troubleshooting section of the README. ([\#7603](#7603)) - Clarifications to the admin api documentation. ([\#7647](#7647)) Internal Changes ---------------- - Convert the identity handler to async/await. ([\#7561](#7561)) - Improve query performance for fetching state from a PostgreSQL database. ([\#7567](#7567)) - Speed up processing of federation stream RDATA rows. ([\#7584](#7584)) - Add comment to systemd example to show postgresql dependency. ([\#7591](#7591)) - Refactor `Ratelimiter` to limit the amount of expensive config value accesses. ([\#7595](#7595)) - Convert groups handlers to async/await. ([\#7600](#7600)) - Clean up exception handling in `SAML2ResponseResource`. ([\#7614](#7614)) - Check that all asynchronous tasks succeed and general cleanup of `MonthlyActiveUsersTestCase` and `TestMauLimit`. ([\#7619](#7619)) - Convert `get_user_id_by_threepid` to async/await. ([\#7620](#7620)) - Switch to upstream `dh-virtualenv` rather than our fork for Debian package builds. ([\#7621](#7621)) - Update CI scripts to check the number in the newsfile fragment. ([\#7623](#7623)) - Check if the localpart of a Matrix ID is reserved for guest users earlier in the registration flow, as well as when responding to requests to `/register/available`. ([\#7625](#7625)) - Minor cleanups to OpenID Connect integration. ([\#7628](#7628)) - Attempt to fix flaky test: `PhoneHomeStatsTestCase.test_performance_100`. ([\#7634](#7634)) - Fix typos of `m.olm.curve25519-aes-sha2` and `m.megolm.v1.aes-sha2` in comments, test files. ([\#7637](#7637)) - Convert user directory, state deltas, and stats handlers to async/await. ([\#7640](#7640)) - Remove some unused constants. ([\#7644](#7644)) - Fix type information on `assert_*_is_admin` methods. ([\#7645](#7645)) - Convert registration handler to async/await. ([\#7649](#7649))

While working on matrix-org#5665 I found myself digging into the `Ratelimiter` class and seeing that it was both: * Rather undocumented, and * causing a *lot* of config checks This PR attempts to refactor and comment the `Ratelimiter` class, as well as encourage config file accesses to only be done at instantiation. Best to be reviewed commit-by-commit.

anoadragon453 added 6 commits May 28, 2020 22:53

Refactor and comment ratelimiting. Set limits in constructor

4f715be

Ratelimiters are instantiated by the HomeServer class

0e6ee7c

This makes it simple for tests to modify/nullify them.

Modify servlets to pull Ratelimiters from HomeServer class

82eac22

Update unittests

a0ef594

lint

6a07c2d

changelog

c322ba0

anoadragon453 requested a review from a team May 28, 2020 22:19

clokep self-assigned this May 29, 2020

clokep previously requested changes May 29, 2020

View reviewed changes

Make rate_hz and burst_count overridable per-request

f6203a6

anoadragon453 force-pushed the anoa/ratelimit_config_perf branch from f93b018 to f6203a6 Compare May 29, 2020 17:35

anoadragon453 added 2 commits June 1, 2020 18:29

Set clock with constructor, store rate_hz per key again

1f6156b

Instantiate Ratelimiters in respective classes

c236806

Other than the registration ratelimiter, as that's used in multiple classes and needs to keep the same state

anoadragon453 force-pushed the anoa/ratelimit_config_perf branch 3 times, most recently from d692e1a to 301965d Compare June 1, 2020 18:13

anoadragon453 added 3 commits June 1, 2020 19:16

Use patch for the Ratelimiter in some tests. Set using config in others

470de6e

Update copyright header

515a186

Remove resolved question

87ab836

anoadragon453 force-pushed the anoa/ratelimit_config_perf branch from 301965d to 87ab836 Compare June 1, 2020 18:16

anoadragon453 added 3 commits June 1, 2020 19:19

lint

56c52a5

lint, mypy

2d7e087

Remove unittest.DEBUG statement

a566b46

anoadragon453 requested a review from clokep June 1, 2020 18:39

clokep reviewed Jun 2, 2020

View reviewed changes

synapse/rest/client/v1/login.py Outdated Show resolved Hide resolved

clokep reviewed Jun 2, 2020

View reviewed changes

changelog.d/7595.misc Outdated Show resolved Hide resolved

Make it obvious that time_now_s is just for testing

d727bed

clokep reviewed Jun 2, 2020

View reviewed changes

synapse/api/ratelimiting.py Outdated Show resolved Hide resolved

anoadragon453 added 2 commits June 2, 2020 19:43

Update ratelimiter calling methods and tests

9f76a8d

No need to re-check for None in can_do_action

8867900

anoadragon453 requested a review from clokep June 2, 2020 18:44

clokep reviewed Jun 2, 2020

View reviewed changes

tests/api/test_ratelimiting.py Outdated Show resolved Hide resolved

anoadragon453 added 3 commits June 3, 2020 13:49

time_now_s is used in ratelimit

ef7383f

Comment changes revolving around time_allowed

189c01b

Fix missed call to self.rate_hz

4a88edb

anoadragon453 force-pushed the anoa/ratelimit_config_perf branch from af13bf6 to da1f6e6 Compare June 3, 2020 14:20

Test Ratelimiter ratelimit method and param overrides

14a0af5

anoadragon453 force-pushed the anoa/ratelimit_config_perf branch from da1f6e6 to 14a0af5 Compare June 3, 2020 14:23

anoadragon453 requested a review from clokep June 3, 2020 14:43

anoadragon453 commented Jun 3, 2020

View reviewed changes

clokep added 2 commits June 4, 2020 11:54

Back out some changes.

c145c81

Do not specify ratelimiters in tests when unnecessary.

12b4d47

clokep force-pushed the anoa/ratelimit_config_perf branch from 1d58e90 to 12b4d47 Compare June 4, 2020 16:14

clokep suggested changes Jun 4, 2020

View reviewed changes

anoadragon453 added 5 commits June 4, 2020 19:21

Update timestamp comment

d84d779

Clean up Exception raising assertion

45a7791

Clean up and split out tests

3899589

Remove _ = style

08c5114

Merge branch 'anoa/ratelimit_config_perf' of github.com:matrix-org/sy…

9beee5f

…napse into anoa/ratelimit_config_perf

anoadragon453 requested a review from clokep June 4, 2020 18:54

clokep approved these changes Jun 4, 2020

View reviewed changes

anoadragon453 merged commit f4e6495 into develop Jun 5, 2020

anoadragon453 deleted the anoa/ratelimit_config_perf branch June 5, 2020 09:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Performance improvements and refactor of Ratelimiter #7595

Performance improvements and refactor of Ratelimiter #7595

anoadragon453 commented May 28, 2020 •

edited

Loading

clokep left a comment

anoadragon453 commented May 29, 2020 •

edited

Loading

anoadragon453 commented Jun 1, 2020

anoadragon453 commented Jun 1, 2020

anoadragon453 Jun 3, 2020 •

edited

Loading

clokep Jun 4, 2020

clokep commented Jun 4, 2020

clokep left a comment

clokep Jun 4, 2020

anoadragon453 Jun 4, 2020

clokep Jun 4, 2020

anoadragon453 Jun 4, 2020

clokep left a comment

Performance improvements and refactor of Ratelimiter #7595

Performance improvements and refactor of Ratelimiter #7595

Conversation

anoadragon453 commented May 28, 2020 • edited Loading

clokep left a comment

Choose a reason for hiding this comment

anoadragon453 commented May 29, 2020 • edited Loading

anoadragon453 commented Jun 1, 2020

anoadragon453 commented Jun 1, 2020

anoadragon453 Jun 3, 2020 • edited Loading

Choose a reason for hiding this comment

clokep Jun 4, 2020

Choose a reason for hiding this comment

clokep commented Jun 4, 2020

clokep left a comment

Choose a reason for hiding this comment

clokep Jun 4, 2020

Choose a reason for hiding this comment

anoadragon453 Jun 4, 2020

Choose a reason for hiding this comment

clokep Jun 4, 2020

Choose a reason for hiding this comment

anoadragon453 Jun 4, 2020

Choose a reason for hiding this comment

clokep left a comment

Choose a reason for hiding this comment

anoadragon453 commented May 28, 2020 •

edited

Loading

anoadragon453 commented May 29, 2020 •

edited

Loading

anoadragon453 Jun 3, 2020 •

edited

Loading