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

Separate creating an event context and persisting the event in the fed handler #9800

Merged
merged 13 commits into from
Apr 14, 2021
Merged

Separate creating an event context and persisting the event in the fed handler #9800

merged 13 commits into from
Apr 14, 2021

Conversation

clokep
Copy link
Member

@clokep clokep commented Apr 12, 2021 •
edited
Loading

This is split out of #9763 as a pure refactoring PR with no behavior changes. It attempts to allow a caller to create the event context without side effects.

  • The bits of _prep_event related to auth checking is moved into do_auth. Note that some of it is moved to the top of do_auth and some to the bottom, to keep the overall flow the same.
  • The call to do_auth is then moved into _handle_new_event and _handle_new_events.
  • This leaves _prep_event as just a call to StateHandler.compute_event_context which is inlined each place that _prep_event was called.
  • Finally, _handle_new_event is modified to take the context in, so any place where _handle_new_event was called also gets a call to StateHandler.compute_event_context before it.
  • This also performs some renames and adds some docstrings:
    • do_auth -> _check_event_auth
    • _handle_new_event(s) -> _auth_and_persist_event(s)

The commits in here kind of became a disaster, but hopefully the overall diff is readable.

@clokep clokep force-pushed the clokep/refactor-fed-handler branch from 35e797d to 0b4f1a8 Compare April 13, 2021 12:35
@clokep clokep requested a review from richvdh April 13, 2021 12:46
richvdh
richvdh reviewed Apr 13, 2021
View reviewed changes
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

some initial thoughts

synapse/handlers/federation.py Outdated Show resolved Hide resolved
synapse/handlers/federation.py Outdated Show resolved Hide resolved
synapse/handlers/federation.py Outdated Show resolved Hide resolved
synapse/handlers/federation.py Show resolved Hide resolved
@richvdh
Copy link
Member

richvdh commented Apr 13, 2021

Hum, it's possible I've failed to understand what you're doing here, having only looked at the first couple of commits. If the above makes no sense, ignore it. I'll have another look at the complete diff.

@richvdh richvdh self-requested a review April 13, 2021 17:09
richvdh
richvdh suggested changes Apr 13, 2021
View reviewed changes
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

Sorry, I've had another look at this. It makes more sense now, but I'm not sure that the split is quite right.

synapse/handlers/federation.py Outdated Show resolved Hide resolved
@clokep clokep requested a review from richvdh April 14, 2021 12:44
@clokep clokep mentioned this pull request Apr 14, 2021
Merged
richvdh
richvdh approved these changes Apr 14, 2021
View reviewed changes
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

generally looks like a good clean-up, I think.

This code is gnarly :/

synapse/handlers/federation.py Outdated Show resolved Hide resolved
synapse/handlers/federation.py Show resolved Hide resolved
synapse/handlers/federation.py Outdated
Comment on lines 2354 to 2356
state:
The state events used to auth the event. If this is not provided
the current state events will be used.
Copy link
Member

Choose a reason for hiding this comment

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

In an ideal world we'd fish this out of the context, but I'm not sure I feel confident enough in the call paths to do that without changing behaviour, so it might be best parked for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had wondered that and decided that made me a bit nervous. 😢

@clokep clokep merged commit 936e698 into develop Apr 14, 2021
@clokep clokep deleted the clokep/refactor-fed-handler branch April 14, 2021 16:35
@clokep clokep mentioned this pull request Apr 14, 2021
Merged
@clokep clokep mentioned this pull request Apr 27, 2021
Merged
anoadragon453 added a commit that referenced this pull request Apr 28, 2021
@anoadragon453
Merge tag 'v1.33.0rc1' into develop
fa6679e 
Synapse 1.33.0rc1 (2021-04-28)
==============================

Features
--------

- Update experimental support for [MSC3083](matrix-org/matrix-spec-proposals#3083): restricting room access via group membership. ([\#9800](#9800), [\#9814](#9814))
- Add experimental support for handling presence on a worker. ([\#9819](#9819), [\#9820](#9820), [\#9828](#9828), [\#9850](#9850))
- Return a new template when an user attempts to renew their account multiple times with the same token, stating that their account is set to expire. This replaces the invalid token template that would previously be shown in this case. This change concerns the optional account validity feature. ([\#9832](#9832))

Bugfixes
--------

- Fixes the OIDC SSO flow when using a `public_baseurl` value including a non-root URL path. ([\#9726](#9726))
- Fix thumbnail generation for some sites with non-standard content types. Contributed by @rkfg. ([\#9788](#9788))
- Add some sanity checks to identity server passed to 3PID bind/unbind endpoints. ([\#9802](#9802))
- Limit the size of HTTP responses read over federation. ([\#9833](#9833))
- Fix a bug which could cause Synapse to get stuck in a loop of resyncing device lists. ([\#9867](#9867))
- Fix a long-standing bug where errors from federation did not propagate to the client. ([\#9868](#9868))

Improved Documentation
----------------------

- Add a note to the docker docs mentioning that we mirror upstream's supported Docker platforms. ([\#9801](#9801))

Internal Changes
----------------

- Add a dockerfile for running Synapse in worker-mode under Complement. ([\#9162](#9162))
- Apply `pyupgrade` across the codebase. ([\#9786](#9786))
- Move some replication processing out of `generic_worker`. ([\#9796](#9796))
- Replace `HomeServer.get_config()` with inline references. ([\#9815](#9815))
- Rename some handlers and config modules to not duplicate the top-level module. ([\#9816](#9816))
- Fix a long-standing bug which caused `max_upload_size` to not be correctly enforced. ([\#9817](#9817))
- Reduce CPU usage of the user directory by reusing existing calculated room membership. ([\#9821](#9821))
- Small speed up for joining large remote rooms. ([\#9825](#9825))
- Introduce flake8-bugbear to the test suite and fix some of its lint violations. ([\#9838](#9838))
- Only store the raw data in the in-memory caches, rather than objects that include references to e.g. the data stores. ([\#9845](#9845))
- Limit length of accepted email addresses. ([\#9855](#9855))
- Remove redundant `synapse.types.Collection` type definition. ([\#9856](#9856))
- Handle recently added rate limits correctly when using `--no-rate-limit` with the demo scripts. ([\#9858](#9858))
- Disable invite rate-limiting by default when running the unit tests. ([\#9871](#9871))
- Pass a reactor into `SynapseSite` to make testing easier. ([\#9874](#9874))
- Make `DomainSpecificString` an `attrs` class. ([\#9875](#9875))
- Add type hints to `synapse.api.auth` and `synapse.api.auth_blocking` modules. ([\#9876](#9876))
- Remove redundant `_PushHTTPChannel` test class. ([\#9878](#9878))
- Remove backwards-compatibility code for Python versions < 3.6. ([\#9879](#9879))
- Small performance improvement around handling new local presence updates. ([\#9887](#9887))
@anoadragon453 anoadragon453 mentioned this pull request Jun 7, 2021
Merged
7 tasks
anoadragon453 added a commit to Sorunome/synapse that referenced this pull request Jun 7, 2021
@anoadragon453
Fix test after do_auth function rename
98367ba 
do_auth was renamed to _check_event_auth in matrix-org#9800.
babolivier added a commit to matrix-org/synapse-dinsic that referenced this pull request Sep 1, 2021
@babolivier
Merge tag 'v1.33.0' into babolivier/dinsic_1.41.0
1d4f5c3 
Synapse 1.33.0 (2021-05-05)
===========================

Features
--------

- Build Debian packages for Ubuntu 21.04 (Hirsute Hippo). ([\#9909](matrix-org/synapse#9909))

Synapse 1.33.0rc2 (2021-04-29)
==============================

Bugfixes
--------

- Fix tight loop when handling presence replication when using workers. Introduced in v1.33.0rc1. ([\#9900](matrix-org/synapse#9900))

Synapse 1.33.0rc1 (2021-04-28)
==============================

Features
--------

- Update experimental support for [MSC3083](matrix-org/matrix-spec-proposals#3083): restricting room access via group membership. ([\#9800](matrix-org/synapse#9800), [\#9814](matrix-org/synapse#9814))
- Add experimental support for handling presence on a worker. ([\#9819](matrix-org/synapse#9819), [\#9820](matrix-org/synapse#9820), [\#9828](matrix-org/synapse#9828), [\#9850](matrix-org/synapse#9850))
- Return a new template when an user attempts to renew their account multiple times with the same token, stating that their account is set to expire. This replaces the invalid token template that would previously be shown in this case. This change concerns the optional account validity feature. ([\#9832](matrix-org/synapse#9832))

Bugfixes
--------

- Fixes the OIDC SSO flow when using a `public_baseurl` value including a non-root URL path. ([\#9726](matrix-org/synapse#9726))
- Fix thumbnail generation for some sites with non-standard content types. Contributed by @rkfg. ([\#9788](matrix-org/synapse#9788))
- Add some sanity checks to identity server passed to 3PID bind/unbind endpoints. ([\#9802](matrix-org/synapse#9802))
- Limit the size of HTTP responses read over federation. ([\#9833](matrix-org/synapse#9833))
- Fix a bug which could cause Synapse to get stuck in a loop of resyncing device lists. ([\#9867](matrix-org/synapse#9867))
- Fix a long-standing bug where errors from federation did not propagate to the client. ([\#9868](matrix-org/synapse#9868))

Improved Documentation
----------------------

- Add a note to the docker docs mentioning that we mirror upstream's supported Docker platforms. ([\#9801](matrix-org/synapse#9801))

Internal Changes
----------------

- Add a dockerfile for running Synapse in worker-mode under Complement. ([\#9162](matrix-org/synapse#9162))
- Apply `pyupgrade` across the codebase. ([\#9786](matrix-org/synapse#9786))
- Move some replication processing out of `generic_worker`. ([\#9796](matrix-org/synapse#9796))
- Replace `HomeServer.get_config()` with inline references. ([\#9815](matrix-org/synapse#9815))
- Rename some handlers and config modules to not duplicate the top-level module. ([\#9816](matrix-org/synapse#9816))
- Fix a long-standing bug which caused `max_upload_size` to not be correctly enforced. ([\#9817](matrix-org/synapse#9817))
- Reduce CPU usage of the user directory by reusing existing calculated room membership. ([\#9821](matrix-org/synapse#9821))
- Small speed up for joining large remote rooms. ([\#9825](matrix-org/synapse#9825))
- Introduce flake8-bugbear to the test suite and fix some of its lint violations. ([\#9838](matrix-org/synapse#9838))
- Only store the raw data in the in-memory caches, rather than objects that include references to e.g. the data stores. ([\#9845](matrix-org/synapse#9845))
- Limit length of accepted email addresses. ([\#9855](matrix-org/synapse#9855))
- Remove redundant `synapse.types.Collection` type definition. ([\#9856](matrix-org/synapse#9856))
- Handle recently added rate limits correctly when using `--no-rate-limit` with the demo scripts. ([\#9858](matrix-org/synapse#9858))
- Disable invite rate-limiting by default when running the unit tests. ([\#9871](matrix-org/synapse#9871))
- Pass a reactor into `SynapseSite` to make testing easier. ([\#9874](matrix-org/synapse#9874))
- Make `DomainSpecificString` an `attrs` class. ([\#9875](matrix-org/synapse#9875))
- Add type hints to `synapse.api.auth` and `synapse.api.auth_blocking` modules. ([\#9876](matrix-org/synapse#9876))
- Remove redundant `_PushHTTPChannel` test class. ([\#9878](matrix-org/synapse#9878))
- Remove backwards-compatibility code for Python versions < 3.6. ([\#9879](matrix-org/synapse#9879))
- Small performance improvement around handling new local presence updates. ([\#9887](matrix-org/synapse#9887))
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@richvdh richvdh richvdh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@clokep @richvdh