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

Refactor `_resolve_state_at_missing_prevs` to return an `EventContext` #13404

Merged

squahtx merged 8 commits into develop from squah/faster_room_joins_refactor_resolve_state_at_missing_prevs

Aug 1, 2022

Contributor

squahtx commented Jul 27, 2022 •

edited

Loading

Previously, _resolve_state_at_missing_prevs returned the resolved
state before an event and a partial state flag. These were unwieldy to
carry around would only ever be used to build an event context. Build
the event context directly instead.

May be easier to review commit by commit.

Sean Quah added 4 commits

July 27, 2022 20:21


          Document exception when compute_event_context() is missing prev events

bd13f9c


          Refactor _check_for_soft_fail to take an EventContext

c1474ec


          Refactor _resolve_state_at_missing_prevs to return an EventContext

5e62cd0


          Add newsfile

2e4b3f4

Signed-off-by: Sean Quah <seanq@matrix.org>

squahtx requested a review from a team as a code owner

July 27, 2022 19:27

squahtx commented

View reviewed changes

synapse/handlers/federation_event.py

Comment on lines 1733 to 1738

                   async def _check_for_soft_fail(
                       self,
                       event: EventBase,
-                      state_ids: Optional[StateMap[str]],
+                      context: EventContext,
                       origin: str,
                   ) -> None:

Contributor Author

squahtx Jul 27, 2022

It only makes sense to call _check_for_soft_fail prior to persisting an event and EventContexts are intended to hold information relevant to persisting an event. So we aren't losing much by accepting an EventContext instead of a StateMap.

synapse/handlers/federation_event.py

@@ @@ -380,7 +377,7 @@ async def on_send_membership_event( @@
                       # need to.
                       await self._event_creation_handler.cache_joined_hosts_for_event(event, context)
-                      await self._check_for_soft_fail(event, None, origin=origin)
+                      await self._check_for_soft_fail(event, context=context, origin=origin)

Contributor Author

squahtx Jul 27, 2022

We have all the prev events here, otherwise we would have raised a RuntimeError when computing the event context. Thus the soft fail check will behave the same as before.

squahtx commented

View reviewed changes

synapse/handlers/federation_event.py

Comment on lines -544 to -570

-                          # There are three possible cases for (state_ids, partial_state):
-                          #   * `state_ids` and `partial_state` are both `None` if we had all the
-                          #     prev_events. The prev_events may or may not have partial state and
-                          #     we won't know until we compute the event context.
-                          #   * `state_ids` is not `None` and `partial_state` is `False` if we were
-                          #     missing some prev_events (but we have full state for any we did
-                          #     have). We calculated the full state after the prev_events.
-                          #   * `state_ids` is not `None` and `partial_state` is `True` if we were
-                          #     missing some, but not all, prev_events. At least one of the
-                          #     prev_events we did have had partial state, so we calculated a partial
-                          #     state after the prev_events.
-                          context = None
-                          if state_ids is not None and partial_state:
-                              # the state after the prev events is still partial. We can't de-partial
-                              # state the event, so don't bother building the event context.
-                              pass
-                          else:
-                              # build a new state group for it if need be
-                              context = await self._state_handler.compute_event_context(
-                                  event,
-                                  state_ids_before_event=state_ids,
-                                  partial_state=partial_state,
-                              )
-                          if context is None or context.partial_state:

Contributor Author

squahtx Jul 27, 2022

We lose the optimization here in exchange for simpler code.

richvdh self-assigned this

richvdh self-requested a review

July 28, 2022 09:56

richvdh reviewed

View reviewed changes

synapse/handlers/federation_event.py Outdated

@@ @@ -872,15 +840,18 @@ async def _process_pulled_event( @@
                           else:
                               raise
-                  async def _resolve_state_at_missing_prevs(
+                  async def _compute_event_context_with_missing_prevs(

Member

richvdh Jul 28, 2022

given we don't know if we have missing prevs, this is a bit of a confusing name (notwithstanding the old name being just as confusing).

Unfortunately I can't think of a better name than _compute_event_context_with_maybe_missing_prevs

synapse/handlers/federation_event.py Outdated

Comment on lines 852 to 854

+                      To build an EventContext, we need to calculate the state at the event. If we
+                      already have all the prev_events for `event`, we can simply use the state at the
+                      prev_events to calculate the state at `event`.

Member

richvdh Jul 28, 2022

s/state at/state before/

synapse/handlers/federation_event.py Outdated

Comment on lines 1161 to 1162

		if context is None:
		context = await self._state_handler.compute_event_context(event)

Member

richvdh Jul 28, 2022

I feel like it would be cleaner to make context mandatory and add a couple of calls to compute_event_context to on_receive_pdu.

synapse/handlers/federation_event.py Outdated Show resolved Hide resolved

richvdh removed their assignment

Sean Quah and others added 4 commits

July 29, 2022 12:02


          fixup: Rename to _compute_event_context_with_maybe_missing_prevs

d513768


          fixup: s/state at/state before/

d13c8ab


          Make context a required argument to on_receive_pdu

da6270b


          Update synapse/handlers/federation_event.py

11e7af1

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

squahtx requested a review from richvdh

July 29, 2022 11:23

Contributor Author

squahtx commented Jul 29, 2022

Thanks for the initial review. I've made the suggested changes.

richvdh approved these changes

View reviewed changes

Member

richvdh left a comment

lgtm!

squahtx merged commit 224d792 into develop

squahtx deleted the squah/faster_room_joins_refactor_resolve_state_at_missing_prevs branch

August 1, 2022 12:53

clokep mentioned this pull request

Fix missing import in federation_event handler. #13431

Merged

clokep added a commit that referenced this pull request


          Fix missing import in federation_event handler. (#13431)

f8e7a94

#13404 removed an import of `Optional` which was still needed
due to #13413 added more usages.

azmeuk pushed a commit to azmeuk/synapse that referenced this pull request


          Refactor _resolve_state_at_missing_prevs to return an EventContext (

3d6f5e3

matrix-org#13404)

Previously, `_resolve_state_at_missing_prevs` returned the resolved
state before an event and a partial state flag. These were unwieldy to
carry around would only ever be used to build an event context. Build
the event context directly instead.

Signed-off-by: Sean Quah <seanq@matrix.org>

azmeuk pushed a commit to azmeuk/synapse that referenced this pull request


          Fix missing import in federation_event handler. (matrix-org#13431)

ca13fb1

matrix-org#13404 removed an import of `Optional` which was still needed
due to matrix-org#13413 added more usages.

Fizzadar added a commit to beeper/synapse-legacy-fork that referenced this pull request


          Merge tag 'v1.65.0' into beeper

84eef5b

Synapse 1.65.0 (2022-08-16)
===========================

No significant changes since 1.65.0rc2.

Synapse 1.65.0rc2 (2022-08-11)
==============================

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

- Revert 'Remove the unspecced `room_id` field in the `/hierarchy` response. ([\matrix-org#13365](matrix-org#13365))' to give more time for clients to update. ([\matrix-org#13501](matrix-org#13501))

Synapse 1.65.0rc1 (2022-08-09)
==============================

Features
--------

- Add support for stable prefixes for [MSC2285 (private read receipts)](matrix-org/matrix-spec-proposals#2285). ([\matrix-org#13273](matrix-org#13273))
- Add new unstable error codes `ORG.MATRIX.MSC3848.ALREADY_JOINED`, `ORG.MATRIX.MSC3848.NOT_JOINED`, and `ORG.MATRIX.MSC3848.INSUFFICIENT_POWER` described in [MSC3848](matrix-org/matrix-spec-proposals#3848). ([\matrix-org#13343](matrix-org#13343))
- Use stable prefixes for [MSC3827](matrix-org/matrix-spec-proposals#3827). ([\matrix-org#13370](matrix-org#13370))
- Add a new module API method to translate a room alias into a room ID. ([\matrix-org#13428](matrix-org#13428))
- Add a new module API method to create a room. ([\matrix-org#13429](matrix-org#13429))
- Add remote join capability to the module API's `update_room_membership` method (in a backwards compatible manner). ([\matrix-org#13441](matrix-org#13441))

Bugfixes
--------

- Update the version of the LDAP3 auth provider module included in the `matrixdotorg/synapse` DockerHub images and the Debian packages hosted on packages.matrix.org to 0.2.2. This version fixes a regression in the module. ([\matrix-org#13470](matrix-org#13470))
- Fix a bug introduced in Synapse v1.41.0 where the `/hierarchy` API returned non-standard information (a `room_id` field under each entry in `children_state`). ([\matrix-org#13365](matrix-org#13365))
- Fix a bug introduced in Synapse 0.24.0 that would respond with the wrong error status code to `/joined_members` requests when the requester is not a current member of the room. Contributed by @andrewdoh. ([\matrix-org#13374](matrix-org#13374))
- Fix bug in handling of typing events for appservices. Contributed by Nick @ Beeper (@Fizzadar). ([\matrix-org#13392](matrix-org#13392))
- Fix a bug introduced in Synapse 1.57.0 where rooms listed in `exclude_rooms_from_sync` in the configuration file would not be properly excluded from incremental syncs. ([\matrix-org#13408](matrix-org#13408))
- Fix a bug in the experimental faster-room-joins support which could cause it to get stuck in an infinite loop. ([\matrix-org#13353](matrix-org#13353))
- Faster room joins: fix a bug which caused rejected events to become un-rejected during state syncing. ([\matrix-org#13413](matrix-org#13413))
- Faster room joins: fix error when running out of servers to sync partial state with, so that Synapse raises the intended error instead. ([\matrix-org#13432](matrix-org#13432))

Updates to the Docker image
---------------------------

- Make Docker images build on armv7 by installing cryptography dependencies in the 'requirements' stage. Contributed by Jasper Spaans. ([\matrix-org#13372](matrix-org#13372))

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

- Update the 'registration tokens' page to acknowledge that the relevant MSC was merged into version 1.2 of the Matrix specification. Contributed by @moan0s. ([\matrix-org#11897](matrix-org#11897))
- Document which HTTP resources support gzip compression. ([\matrix-org#13221](matrix-org#13221))
- Add steps describing how to elevate an existing user to administrator by manipulating the database. ([\matrix-org#13230](matrix-org#13230))
- Fix wrong headline for `url_preview_accept_language` in documentation. ([\matrix-org#13437](matrix-org#13437))
- Remove redundant 'Contents' section from the Configuration Manual. Contributed by @dklimpel. ([\matrix-org#13438](matrix-org#13438))
- Update documentation for config setting `macaroon_secret_key`. ([\matrix-org#13443](matrix-org#13443))
- Update outdated information on `sso_mapping_providers` documentation. ([\matrix-org#13449](matrix-org#13449))
- Fix example code in module documentation of `password_auth_provider_callbacks`. ([\matrix-org#13450](matrix-org#13450))
- Make the configuration for the cache clearer. ([\matrix-org#13481](matrix-org#13481))

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

- Extend the release script to automatically push a new SyTest branch, rather than having that be a manual process. ([\matrix-org#12978](matrix-org#12978))
- Make minor clarifications to the error messages given when we fail to join a room via any server. ([\matrix-org#13160](matrix-org#13160))
- Enable Complement CI tests in the 'latest deps' test run. ([\matrix-org#13213](matrix-org#13213))
- Fix long-standing bugged logic which was never hit in `get_pdu` asking every remote destination even after it finds an event. ([\matrix-org#13346](matrix-org#13346))
- Faster room joins: avoid blocking when pulling events with partially missing prev events. ([\matrix-org#13355](matrix-org#13355))
- Instrument `/messages` for understandable traces in Jaeger. ([\matrix-org#13368](matrix-org#13368))
- Remove an unused argument to `get_relations_for_event`. ([\matrix-org#13383](matrix-org#13383))
- Add a `merge-back` command to the release script, which automates merging the correct branches after a release. ([\matrix-org#13393](matrix-org#13393))
- Adding missing type hints to tests. ([\matrix-org#13397](matrix-org#13397))
- Faster Room Joins: don't leave a stuck room partial state flag if the join fails. ([\matrix-org#13403](matrix-org#13403))
- Refactor `_resolve_state_at_missing_prevs` to compute an `EventContext` instead. ([\matrix-org#13404](matrix-org#13404), [\matrix-org#13431](matrix-org#13431))
- Faster Room Joins: prevent Synapse from answering federated join requests for a room which it has not fully joined yet. ([\matrix-org#13416](matrix-org#13416))
- Re-enable running Complement tests against Synapse with workers. ([\matrix-org#13420](matrix-org#13420))
- Prevent unnecessary lookups to any external `get_event` cache. Contributed by Nick @ Beeper (@Fizzadar). ([\matrix-org#13435](matrix-org#13435))
- Add some tracing to give more insight into local room joins. ([\matrix-org#13439](matrix-org#13439))
- Rename class `RateLimitConfig` to `RatelimitSettings` and `FederationRateLimitConfig` to `FederationRatelimitSettings`. ([\matrix-org#13442](matrix-org#13442))
- Add some comments about how event push actions are stored. ([\matrix-org#13445](matrix-org#13445), [\matrix-org#13455](matrix-org#13455))
- Improve rebuild speed for the "synapse-workers" docker image. ([\matrix-org#13447](matrix-org#13447))
- Fix `@tag_args` being off-by-one with the arguments when tagging a span (tracing). ([\matrix-org#13452](matrix-org#13452))
- Update type of `EventContext.rejected`. ([\matrix-org#13460](matrix-org#13460))
- Use literals in place of `HTTPStatus` constants in tests. ([\matrix-org#13463](matrix-org#13463), [\matrix-org#13469](matrix-org#13469))
- Correct a misnamed argument in state res v2 internals. ([\matrix-org#13467](matrix-org#13467))

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet