Change @cache_in_self to use underscore-prefixed attributes #8565

ShadowJonathan · 2020-10-16T13:03:28Z

also change some usages to use .get_xyz() instead of .xyz (except for some usages that explicitly set the attribute)

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 <jonathan@automatia.nl>

also change some usages to use .get_xyz() instead of .xyz

ShadowJonathan · 2020-10-16T13:19:11Z

(Please take a close look at this one, as it's pretty much deliberately breaking, i've already run tox to try some scream tests, but i'll not be sure if i've hit every spot everywhere, it'd likely cause None-related errors, or "cannot find attribute" errors, probably combined)

richvdh

this is moderately terrifying, but seems sensible.

I am led to wonder though: should we be storing the attributes in a dedicated dict rather than abusing attributes within the Homeserver object?

changelog.d/8565.misc

synapse/server.py

richvdh · 2020-10-19T17:46:04Z

I ran

grep -r -P '\bhs\.(?!get|config|is_mine|hostname|should_send_federation|signing_key|remove_pusher|version_string)' synapse

... and it didn't find anything beyond what you already found.

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

ShadowJonathan · 2020-10-19T18:05:32Z

I am led to wonder though: should we be storing the attributes in a dedicated dict rather than abusing attributes within the Homeserver object?

Would this be more performant? I like the idea, but the reason this is in place to begin with is to have better cache performance.

richvdh · 2020-10-19T20:03:19Z

Would this be more performant?

Well no, because we would first have to fetch the component dict (the equivalent of the existing getattr) and then retrieve the component from the dict. (Though doing so might open the path to making HomeServer use __slots__, which would offset the penalty somewhat.) However:

I like the idea, but the reason this is in place to begin with is to have better cache performance.

sorry, by "this" do you mean @cache_in_self? If so, that's not correct: the reason it's in place is to provide an easy way to (a) replace components during tests, and (b) provide each component with the dependencies it requires without having to worry about the correct order to initialise things in. In short, it's a form of dependency injection.

By definition, all the objects returned by @cache_in_self are static: we can fetch them once, up front. If we're using hs.get_foo() anywhere in performance critical code, that ought to be factored out.

synapse/handlers/identity.py

synapse/server.py

tests/rest/client/v1/test_presence.py

clokep · 2020-10-27T15:18:56Z

@ShadowJonathan There was a couple more comments in here which were missed. Not sure if they were ignored on purpose or not.

ShadowJonathan · 2020-10-27T17:11:16Z

@clokep whoops, i didn't see them as direct calls for changes at the time, and i didnt think I had a call in the matter, I'll look again in a sec

clokep

I think this looks fine. I did have one final question though. See the comments.

clokep · 2020-11-16T19:50:55Z

tests/replication/_base.py

@@ -69,7 +69,7 @@ def prepare(self, reactor, clock, hs):
        self.worker_hs.get_datastore().db_pool = hs.get_datastore().db_pool

        self.test_handler = self._build_replication_data_handler()
-        self.worker_hs.replication_data_handler = self.test_handler
+        self.worker_hs._replication_data_handler = self.test_handler


Can replication_data_handler be passed into setup_test_homeserver (similar to the changes made bleow in tests/rest/client/v1/test_presence.py)?

This cannot be easily done because _build_replication_data_handler instantiates TestReplicationDataHandler with the homeserver, so you end up with a chicken/egg problem.

changelog.d/8565.misc

clokep · 2020-11-30T18:28:50Z

Thanks! 👍

This test was broken by #8565. It doesn't need to set set `self.clock` here anyway - that is done by `setUp`.

ShadowJonathan · 2020-12-02T18:21:51Z

@clokep crap, sorry i wasnt here on time, ive had a tumultuous few last weeks

clokep · 2020-12-02T18:28:27Z

@clokep crap, sorry i wasnt here on time, ive had a tumultuous few last weeks

Don't worry about it! Figured it was easy enough to just push over the finish line. 👍

@angdraug

Synapse 1.24.0 (2020-12-09) =========================== Due to the two security issues highlighted below, server administrators are encouraged to update Synapse. We are not aware of these vulnerabilities being exploited in the wild. Security advisory ----------------- The following issues are fixed in v1.23.1 and v1.24.0. - There is a denial of service attack ([CVE-2020-26257](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-26257)) against the federation APIs in which future events will not be correctly sent to other servers over federation. This affects all servers that participate in open federation. (Fixed in [#8776](matrix-org/synapse#8776)). - Synapse may be affected by OpenSSL [CVE-2020-1971](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-1971). Synapse administrators should ensure that they have the latest versions of the cryptography Python package installed. To upgrade Synapse along with the cryptography package: * Administrators using the [`matrix.org` Docker image](https://hub.docker.com/r/matrixdotorg/synapse/) or the [Debian/Ubuntu packages from `matrix.org`](https://github.com/matrix-org/synapse/blob/master/INSTALL.md#matrixorg-packages) should ensure that they have version 1.24.0 or 1.23.1 installed: these images include the updated packages. * Administrators who have [installed Synapse from source](https://github.com/matrix-org/synapse/blob/master/INSTALL.md#installing-from-source) should upgrade the cryptography package within their virtualenv by running: ```sh <path_to_virtualenv>/bin/pip install 'cryptography>=3.3' ``` * Administrators who have installed Synapse from distribution packages should consult the information from their distributions. Internal Changes ---------------- - Add a maximum version for pysaml2 on Python 3.5. ([\#8898](matrix-org/synapse#8898)) Synapse 1.24.0rc2 (2020-12-04) ============================== Bugfixes -------- - Fix a regression in v1.24.0rc1 which failed to allow SAML mapping providers which were unable to redirect users to an additional page. ([\#8878](matrix-org/synapse#8878)) Internal Changes ---------------- - Add support for the `prometheus_client` newer than 0.9.0. Contributed by Jordan Bancino. ([\#8875](matrix-org/synapse#8875)) Synapse 1.24.0rc1 (2020-12-02) ============================== Features -------- - Add admin API for logging in as a user. ([\#8617](matrix-org/synapse#8617)) - Allow specification of the SAML IdP if the metadata returns multiple IdPs. ([\#8630](matrix-org/synapse#8630)) - Add support for re-trying generation of a localpart for OpenID Connect mapping providers. ([\#8801](matrix-org/synapse#8801), [\#8855](matrix-org/synapse#8855)) - Allow the `Date` header through CORS. Contributed by Nicolas Chamo. ([\#8804](matrix-org/synapse#8804)) - Add a config option, `push.group_by_unread_count`, which controls whether unread message counts in push notifications are defined as "the number of rooms with unread messages" or "total unread messages". ([\#8820](matrix-org/synapse#8820)) - Add `force_purge` option to delete-room admin api. ([\#8843](matrix-org/synapse#8843)) Bugfixes -------- - Fix a bug where appservices may be sent an excessive amount of read receipts and presence. Broke in v1.22.0. ([\#8744](matrix-org/synapse#8744)) - Fix a bug in some federation APIs which could lead to unexpected behaviour if different parameters were set in the URI and the request body. ([\#8776](matrix-org/synapse#8776)) - Fix a bug where synctl could spawn duplicate copies of a worker. Contributed by Waylon Cude. ([\#8798](matrix-org/synapse#8798)) - Allow per-room profiles to be used for the server notice user. ([\#8799](matrix-org/synapse#8799)) - Fix a bug where logging could break after a call to SIGHUP. ([\#8817](matrix-org/synapse#8817)) - Fix `register_new_matrix_user` failing with "Bad Request" when trailing slash is included in server URL. Contributed by @angdraug. ([\#8823](matrix-org/synapse#8823)) - Fix a minor long-standing bug in login, where we would offer the `password` login type if a custom auth provider supported it, even if password login was disabled. ([\#8835](matrix-org/synapse#8835)) - Fix a long-standing bug which caused Synapse to require unspecified parameters during user-interactive authentication. ([\#8848](matrix-org/synapse#8848)) - Fix a bug introduced in v1.20.0 where the user-agent and IP address reported during user registration for CAS, OpenID Connect, and SAML were of the wrong form. ([\#8784](matrix-org/synapse#8784)) Improved Documentation ---------------------- - Clarify the usecase for a msisdn delegate. Contributed by Adrian Wannenmacher. ([\#8734](matrix-org/synapse#8734)) - Remove extraneous comma from JSON example in User Admin API docs. ([\#8771](matrix-org/synapse#8771)) - Update `turn-howto.md` with troubleshooting notes. ([\#8779](matrix-org/synapse#8779)) - Fix the example on how to set the `Content-Type` header in nginx for the Client Well-Known URI. ([\#8793](matrix-org/synapse#8793)) - Improve the documentation for the admin API to list all media in a room with respect to encrypted events. ([\#8795](matrix-org/synapse#8795)) - Update the formatting of the `push` section of the homeserver config file to better align with the [code style guidelines](https://github.com/matrix-org/synapse/blob/develop/docs/code_style.md#configuration-file-format). ([\#8818](matrix-org/synapse#8818)) - Improve documentation how to configure prometheus for workers. ([\#8822](matrix-org/synapse#8822)) - Update example prometheus console. ([\#8824](matrix-org/synapse#8824)) Deprecations and Removals ------------------------- - Remove old `/_matrix/client/*/admin` endpoints which were deprecated since Synapse 1.20.0. ([\#8785](matrix-org/synapse#8785)) - Disable pretty printing JSON responses for curl. Users who want pretty-printed output should use [jq](https://stedolan.github.io/jq/) in combination with curl. Contributed by @tulir. ([\#8833](matrix-org/synapse#8833)) Internal Changes ---------------- - Simplify the way the `HomeServer` object caches its internal attributes. ([\#8565](matrix-org/synapse#8565), [\#8851](matrix-org/synapse#8851)) - Add an example and documentation for clock skew to the SAML2 sample configuration to allow for clock/time difference between the homserver and IdP. Contributed by @localguru. ([\#8731](matrix-org/synapse#8731)) - Generalise `RoomMemberHandler._locally_reject_invite` to apply to more flows than just invite. ([\#8751](matrix-org/synapse#8751)) - Generalise `RoomStore.maybe_store_room_on_invite` to handle other, non-invite membership events. ([\#8754](matrix-org/synapse#8754)) - Refactor test utilities for injecting HTTP requests. ([\#8757](matrix-org/synapse#8757), [\#8758](matrix-org/synapse#8758), [\#8759](matrix-org/synapse#8759), [\#8760](matrix-org/synapse#8760), [\#8761](matrix-org/synapse#8761), [\#8777](matrix-org/synapse#8777)) - Consolidate logic between the OpenID Connect and SAML code. ([\#8765](matrix-org/synapse#8765)) - Use `TYPE_CHECKING` instead of magic `MYPY` variable. ([\#8770](matrix-org/synapse#8770)) - Add a commandline script to sign arbitrary json objects. ([\#8772](matrix-org/synapse#8772)) - Minor log line improvements for the SSO mapping code used to generate Matrix IDs from SSO IDs. ([\#8773](matrix-org/synapse#8773)) - Add additional error checking for OpenID Connect and SAML mapping providers. ([\#8774](matrix-org/synapse#8774), [\#8800](matrix-org/synapse#8800)) - Add type hints to HTTP abstractions. ([\#8806](matrix-org/synapse#8806), [\#8812](matrix-org/synapse#8812)) - Remove unnecessary function arguments and add typing to several membership replication classes. ([\#8809](matrix-org/synapse#8809)) - Optimise the lookup for an invite from another homeserver when trying to reject it. ([\#8815](matrix-org/synapse#8815)) - Add tests for `password_auth_provider`s. ([\#8819](matrix-org/synapse#8819)) - Drop redundant database index on `event_json`. ([\#8845](matrix-org/synapse#8845)) - Simplify `uk.half-shot.msc2778.login.application_service` login handler. ([\#8847](matrix-org/synapse#8847)) - Refactor `password_auth_provider` support code. ([\#8849](matrix-org/synapse#8849)) - Add missing `ordering` to background database updates. ([\#8850](matrix-org/synapse#8850)) - Allow for specifying a room version when creating a room in unit tests via `RestHelper.create_room_as`. ([\#8854](matrix-org/synapse#8854))

ShadowJonathan added 2 commits October 16, 2020 15:02

change @cache_in_self to use underscore-prefixed attributes

c392125

also change some usages to use .get_xyz() instead of .xyz

add news

d7e24a2

anoadragon453 requested a review from a team October 16, 2020 13:55

richvdh reviewed Oct 19, 2020

View reviewed changes

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

synapse/server.py Outdated Show resolved Hide resolved

grammar correction

ade638a

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

Make clock and registration_ratelimiter a cached attribute

a0ccb30

ShadowJonathan requested a review from richvdh October 21, 2020 14:22

clokep reviewed Oct 21, 2020

View reviewed changes

synapse/handlers/identity.py Show resolved Hide resolved

clokep reviewed Oct 21, 2020

View reviewed changes

synapse/server.py Outdated Show resolved Hide resolved

clokep reviewed Oct 21, 2020

View reviewed changes

tests/rest/client/v1/test_presence.py Outdated Show resolved Hide resolved

ShadowJonathan added 2 commits October 21, 2020 21:55

change ph to presence_handler

b2f0793

Please The Linter

265b6e4

clokep removed the request for review from richvdh October 27, 2020 15:19

clokep added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Oct 27, 2020

Apply feedback

bca1e49

ShadowJonathan requested a review from clokep November 2, 2020 08:15

erikjohnston removed the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Nov 2, 2020

clokep requested review from a team and removed request for clokep November 2, 2020 18:01

clokep approved these changes Nov 16, 2020

View reviewed changes

Clarify changelog.

c950d7e

clokep merged commit ca60822 into matrix-org:develop Nov 30, 2020

richvdh added a commit that referenced this pull request Dec 1, 2020

Fix broken testcase

0177544

This test was broken by #8565. It doesn't need to set set `self.clock` here anyway - that is done by `setUp`.

richvdh mentioned this pull request Dec 1, 2020

Fix broken testcase #8851

Merged

richvdh added a commit that referenced this pull request Dec 1, 2020

Fix broken testcase (#8851)

09ac056

This test was broken by #8565. It doesn't need to set set `self.clock` here anyway - that is done by `setUp`.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change @cache_in_self to use underscore-prefixed attributes #8565

Change @cache_in_self to use underscore-prefixed attributes #8565

ShadowJonathan commented Oct 16, 2020 •

edited

Loading

ShadowJonathan commented Oct 16, 2020 •

edited

Loading

richvdh left a comment

richvdh commented Oct 19, 2020

ShadowJonathan commented Oct 19, 2020

richvdh commented Oct 19, 2020

clokep commented Oct 27, 2020

ShadowJonathan commented Oct 27, 2020

clokep left a comment

clokep Nov 16, 2020

clokep Nov 30, 2020

clokep commented Nov 30, 2020

ShadowJonathan commented Dec 2, 2020

clokep commented Dec 2, 2020

Change @cache_in_self to use underscore-prefixed attributes #8565

Change @cache_in_self to use underscore-prefixed attributes #8565

Conversation

ShadowJonathan commented Oct 16, 2020 • edited Loading

Pull Request Checklist

ShadowJonathan commented Oct 16, 2020 • edited Loading

richvdh left a comment

Choose a reason for hiding this comment

richvdh commented Oct 19, 2020

ShadowJonathan commented Oct 19, 2020

richvdh commented Oct 19, 2020

clokep commented Oct 27, 2020

ShadowJonathan commented Oct 27, 2020

clokep left a comment

Choose a reason for hiding this comment

clokep Nov 16, 2020

Choose a reason for hiding this comment

clokep Nov 30, 2020

Choose a reason for hiding this comment

clokep commented Nov 30, 2020

ShadowJonathan commented Dec 2, 2020

clokep commented Dec 2, 2020

ShadowJonathan commented Oct 16, 2020 •

edited

Loading

ShadowJonathan commented Oct 16, 2020 •

edited

Loading