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

Faster joins: Refactor handling of servers in room #14954

Merged
merged 4 commits into from
Feb 3, 2023
Merged

Faster joins: Refactor handling of servers in room #14954

merged 4 commits into from
Feb 3, 2023

Conversation

squahtx
Copy link
Contributor

@squahtx squahtx commented Jan 31, 2023

Ensure that the list of servers in a partial state room always contains
the server we joined off.

Also refactor get_partial_state_servers_at_join to return None when
the given room is no longer partial stated, to explicitly indicate when
the room has partial state. Otherwise it's not clear whether an empty
list means that the room has full state, or the room is partial stated,
but the server we joined off told us that there are no servers in the
room.

Signed-off-by: Sean Quah [email protected]

Sean Quah added 3 commits January 31, 2023 01:25
Ensure list of servers in partial state room contains original server
07853dd 
...and hence is never empty for a partial state room. This couldn't
happen previously because we check if the list is falsy, but let's make
it more explicit and intentional.

Signed-off-by: Sean Quah <[email protected]>
Refactor get_partial_state_servers_at_join
2ec2796 
Return `None` when the given room is no longer partial stated, to
explicitly indicate when the room has partial state. Otherwise it's not
clear whether an empty list means that the room has full state, or the
room is partial stated, but the server we joined off told us that there
are no servers in the room.

Signed-off-by: Sean Quah <[email protected]>
Add newsfile
f65d213
@squahtx squahtx requested a review from a team as a code owner January 31, 2023 16:59
squahtx
squahtx commented Jan 31, 2023
View reviewed changes
synapse/federation/sender/__init__.py
@@ -447,7 +447,7 @@ async def handle_event(event: EventBase) -> None:
)
)

if len(partial_state_destinations) > 0:
if partial_state_destinations is not None:
Copy link
Contributor Author

@squahtx squahtx Jan 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I initially thought there was a bug here, where we would block on the full state of the room (below) when the server we joined off returned an empty list of servers in the room. But while writing this PR to fix the bug, I discovered that we validate that the list is truthy and so the bug can't happen.

clokep
clokep reviewed Feb 1, 2023
View reviewed changes
synapse/federation/federation_client.py Outdated
Comment on lines 1163 to 1166
if destination not in servers_in_room:
# `servers_in_room` is supposed to be a complete list.
# Fix things up if the remote homeserver is badly behaved.
servers_in_room = [destination] + servers_in_room
Copy link
Member

Choose a reason for hiding this comment

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

Should we be making this a set? What if the remote server returns the same thing for servers_in_room 100 times?

Is it possible for destination to no longer be in the room at this point? (I'm guessing no because we just got a response from it?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question. We'll fail to insert the list into the database, because there's a unique constraint. And the join will fail.

There's nothing stopping destination from leaving the room immediately after our join really.
If destination leaves the room, we'll try syncing state from other servers in the list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Representing the server list as a set sounds like a good idea, so I did it in 163c68c.

clokep
clokep reviewed Feb 1, 2023
View reviewed changes
synapse/federation/federation_client.py
Comment on lines +1168 to +1171
if not partial_state:
raise InvalidResponseError(
"members_omitted was set, but we asked for full state"
)
Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend moving this above the destination check to have all the error checking up-front.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 163c68c.

clokep
clokep reviewed Feb 1, 2023
View reviewed changes
synapse/handlers/device.py
@@ -859,6 +859,7 @@ async def handle_room_un_partial_stated(self, room_id: str) -> None:
known_hosts_at_join = await self.store.get_partial_state_servers_at_join(
room_id
)
assert known_hosts_at_join is not None
Copy link
Member

Choose a reason for hiding this comment

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

Should this be an error? Usually assertions are used for programming errors, but this seem to be input validation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The assert can't (shouldn't?) ever be hit unless we've introduced a bug. This method is only run when we're about to finish syncing state. At that point the room is still partial stated, which implies we have a list of servers in it.

clokep
clokep reviewed Feb 1, 2023
View reviewed changes
changelog.d/14954.misc
@@ -0,0 +1 @@
Faster room joins: Refactor internal handling of servers in room to never store an empty list.
Copy link
Member

Choose a reason for hiding this comment

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

Is this fixing a bug? A potential bug? Or just clean-up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's somewhere in between fixing a potential bug and a clean up. Probably closer to a clean up.

@squahtx squahtx requested a review from clokep February 3, 2023 00:00
@squahtx squahtx merged commit 0a686d1 into develop Feb 3, 2023
@squahtx squahtx deleted the squah/faster_room_joins_refactor_servers_in_room branch February 3, 2023 15:40
Fizzadar added a commit to Fizzadar/synapse that referenced this pull request Feb 20, 2023
@Fizzadar
Merge tag 'v1.77.0' into beeper
120c50b 
Synapse 1.77.0 (2023-02-14)
===========================

No significant changes since 1.77.0rc2.

Synapse 1.77.0rc2 (2023-02-10)
==============================

Bugfixes
--------

- Fix bug where retried replication requests would return a failure. Introduced in v1.76.0. ([\matrix-org#15024](matrix-org#15024))

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

- Prepare for future database schema changes. ([\matrix-org#15036](matrix-org#15036))

Synapse 1.77.0rc1 (2023-02-07)
==============================

Features
--------

- Experimental support for [MSC3952](matrix-org/matrix-spec-proposals#3952): intentional mentions. ([\matrix-org#14823](matrix-org#14823), [\matrix-org#14943](matrix-org#14943), [\matrix-org#14957](matrix-org#14957), [\matrix-org#14958](matrix-org#14958))
- Experimental support to suppress notifications from message edits ([MSC3958](matrix-org/matrix-spec-proposals#3958)). ([\matrix-org#14960](matrix-org#14960), [\matrix-org#15016](matrix-org#15016))
- Add profile information, devices and connections to the command line [user data export tool](https://matrix-org.github.io/synapse/v1.77/usage/administration/admin_faq.html#how-can-i-export-user-data). ([\matrix-org#14894](matrix-org#14894))
- Improve performance when joining or sending an event in large rooms. ([\matrix-org#14962](matrix-org#14962))
- Improve performance of joining and leaving large rooms with many local users. ([\matrix-org#14971](matrix-org#14971))

Bugfixes
--------

- Fix a bug introduced in Synapse 1.53.0 where `next_batch` tokens from `/sync` could not be used with the `/relations` endpoint. ([\matrix-org#14866](matrix-org#14866))
- Fix a bug introduced in Synapse 1.35.0 where the module API's `send_local_online_presence_to` would fail to send presence updates over federation. ([\matrix-org#14880](matrix-org#14880))
- Fix a bug introduced in Synapse 1.70.0 where the background updates to add non-thread unique indexes on receipts could fail when upgrading from 1.67.0 or earlier. ([\matrix-org#14915](matrix-org#14915))
- Fix a regression introduced in Synapse 1.69.0 which can result in database corruption when database migrations are interrupted on sqlite. ([\matrix-org#14926](matrix-org#14926))
- Fix a bug introduced in Synapse 1.68.0 where we were unable to service remote joins in rooms with `@room` notification levels set to `null` in their (malformed) power levels. ([\matrix-org#14942](matrix-org#14942))
- Fix a bug introduced in Synapse 1.64.0 where boolean power levels were erroneously permitted in [v10 rooms](https://spec.matrix.org/v1.5/rooms/v10/). ([\matrix-org#14944](matrix-org#14944))
- Fix a long-standing bug where sending messages on servers with presence enabled would spam "Re-starting finished log context" log lines. ([\matrix-org#14947](matrix-org#14947))
- Fix a bug introduced in Synapse 1.68.0 where logging from the Rust module was not properly logged. ([\matrix-org#14976](matrix-org#14976))
- Fix various long-standing bugs in Synapse's config, event and request handling where booleans were unintentionally accepted where an integer was expected. ([\matrix-org#14945](matrix-org#14945))

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

- Add missing type hints. ([\matrix-org#14879](matrix-org#14879), [\matrix-org#14886](matrix-org#14886), [\matrix-org#14887](matrix-org#14887), [\matrix-org#14904](matrix-org#14904), [\matrix-org#14927](matrix-org#14927), [\matrix-org#14956](matrix-org#14956), [\matrix-org#14983](matrix-org#14983), [\matrix-org#14984](matrix-org#14984), [\matrix-org#14985](matrix-org#14985), [\matrix-org#14987](matrix-org#14987), [\matrix-org#14988](matrix-org#14988), [\matrix-org#14990](matrix-org#14990), [\matrix-org#14991](matrix-org#14991), [\matrix-org#14992](matrix-org#14992), [\matrix-org#15007](matrix-org#15007))
- Use `StrCollection` to avoid potential bugs with `Collection[str]`. ([\matrix-org#14922](matrix-org#14922))
- Allow running the complement tests suites with the asyncio reactor enabled. ([\matrix-org#14858](matrix-org#14858))
- Improve performance of `/sync` in a few situations. ([\matrix-org#14908](matrix-org#14908), [\matrix-org#14970](matrix-org#14970))
- Document how to handle Dependabot pull requests. ([\matrix-org#14916](matrix-org#14916))
- Fix typo in release script. ([\matrix-org#14920](matrix-org#14920))
- Update build system requirements to allow building with poetry-core 1.5.0. ([\matrix-org#14949](matrix-org#14949), [\matrix-org#15019](matrix-org#15019))
- Add an [lnav](https://lnav.org) config file for Synapse logs to `/contrib/lnav`. ([\matrix-org#14953](matrix-org#14953))
- Faster joins: Refactor internal handling of servers in room to never store an empty list. ([\matrix-org#14954](matrix-org#14954))
- Faster joins: tag `v2/send_join/` requests to indicate if they served a partial join response. ([\matrix-org#14950](matrix-org#14950))
- Allow running `cargo` without the `extension-module` option. ([\matrix-org#14965](matrix-org#14965))
- Preparatory work for adding a denormalised event stream ordering column in the future. Contributed by Nick @ Beeper (@Fizzadar). ([\matrix-org#14979](matrix-org#14979), [9cd7610](matrix-org@9cd7610), [f10caa7](matrix-org@f10caa7); see [\matrix-org#15014](matrix-org#15014))
- Add tests for `_flatten_dict`. ([\matrix-org#14981](matrix-org#14981), [\matrix-org#15002](matrix-org#15002))

<details><summary>Dependabot updates</summary>

- Bump dtolnay/rust-toolchain from e645b0cf01249a964ec099494d38d2da0f0b349f to 9cd00a88a73addc8617065438eff914dd08d0955. ([\matrix-org#14968](matrix-org#14968))
- Bump docker/build-push-action from 3 to 4. ([\matrix-org#14952](matrix-org#14952))
- Bump ijson from 3.1.4 to 3.2.0.post0. ([\matrix-org#14935](matrix-org#14935))
- Bump types-pyyaml from 6.0.12.2 to 6.0.12.3. ([\matrix-org#14936](matrix-org#14936))
- Bump types-jsonschema from 4.17.0.2 to 4.17.0.3. ([\matrix-org#14937](matrix-org#14937))
- Bump types-pillow from 9.4.0.3 to 9.4.0.5. ([\matrix-org#14938](matrix-org#14938))
- Bump hiredis from 2.0.0 to 2.1.1. ([\matrix-org#14939](matrix-org#14939))
- Bump hiredis from 2.1.1 to 2.2.1. ([\matrix-org#14993](matrix-org#14993))
- Bump types-setuptools from 65.6.0.3 to 67.1.0.0. ([\matrix-org#14994](matrix-org#14994))
- Bump prometheus-client from 0.15.0 to 0.16.0. ([\matrix-org#14995](matrix-org#14995))
- Bump anyhow from 1.0.68 to 1.0.69. ([\matrix-org#14996](matrix-org#14996))
- Bump serde_json from 1.0.91 to 1.0.92. ([\matrix-org#14997](matrix-org#14997))
- Bump isort from 5.11.4 to 5.11.5. ([\matrix-org#14998](matrix-org#14998))
- Bump phonenumbers from 8.13.4 to 8.13.5. ([\matrix-org#14999](matrix-org#14999))
</details>

# -----BEGIN PGP SIGNATURE-----
#
# iHUEABYKAB0WIQSTI7xPaHQ1yo0PA8uSL1esuTqr+QUCY+ubcgAKCRCSL1esuTqr
# +foKAP9K8HQeGlOns6GRRiyY1EPILRvptAXeMit2eQ19J+ROKAD+JZM5WqlpWAdW
# ikmC4GV8hps01IAWFwKtK3+pLqg79gc=
# =yBT7
# -----END PGP SIGNATURE-----
# gpg: Signature made Tue Feb 14 14:32:18 2023 GMT
# gpg:                using EDDSA key 9323BC4F687435CA8D0F03CB922F57ACB93AABF9
# gpg: Can't check signature: No public key

# Conflicts:
#	docker/Dockerfile
#	poetry.lock
#	rust/src/push/base_rules.rs
#	rust/src/push/evaluator.rs
#	rust/src/push/mod.rs
#	synapse/config/experimental.py
#	synapse/event_auth.py
#	synapse/handlers/message.py
#	synapse/handlers/pagination.py
#	synapse/push/bulk_push_rule_evaluator.py
#	synapse/rest/admin/rooms.py
#	synapse/storage/databases/main/devices.py
#	synapse/storage/databases/main/roommember.py
#	tests/push/test_push_rule_evaluator.py
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Mar 1, 2023
@Midar
Update chat/matrix-synapse to 1.77.0
1cf80f5 
Synapse 1.77.0 (2023-02-14)
===========================

No significant changes since 1.77.0rc2.


Synapse 1.77.0rc2 (2023-02-10)
==============================

Bugfixes
--------

- Fix bug where retried replication requests would return a failure. Introduced in v1.76.0. ([\#15024](matrix-org/synapse#15024))


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

- Prepare for future database schema changes. ([\#15036](matrix-org/synapse#15036))


Synapse 1.77.0rc1 (2023-02-07)
==============================

Features
--------

- Experimental support for [MSC3952](matrix-org/matrix-spec-proposals#3952): intentional mentions. ([\#14823](matrix-org/synapse#14823), [\#14943](matrix-org/synapse#14943), [\#14957](matrix-org/synapse#14957), [\#14958](matrix-org/synapse#14958))
- Experimental support to suppress notifications from message edits ([MSC3958](matrix-org/matrix-spec-proposals#3958)). ([\#14960](matrix-org/synapse#14960), [\#15016](matrix-org/synapse#15016))
- Add profile information, devices and connections to the command line [user data export tool](https://matrix-org.github.io/synapse/v1.77/usage/administration/admin_faq.html#how-can-i-export-user-data). ([\#14894](matrix-org/synapse#14894))
- Improve performance when joining or sending an event in large rooms. ([\#14962](matrix-org/synapse#14962))
- Improve performance of joining and leaving large rooms with many local users. ([\#14971](matrix-org/synapse#14971))


Bugfixes
--------

- Fix a bug introduced in Synapse 1.53.0 where `next_batch` tokens from `/sync` could not be used with the `/relations` endpoint. ([\#14866](matrix-org/synapse#14866))
- Fix a bug introduced in Synapse 1.35.0 where the module API's `send_local_online_presence_to` would fail to send presence updates over federation. ([\#14880](matrix-org/synapse#14880))
- Fix a bug introduced in Synapse 1.70.0 where the background updates to add non-thread unique indexes on receipts could fail when upgrading from 1.67.0 or earlier. ([\#14915](matrix-org/synapse#14915))
- Fix a regression introduced in Synapse 1.69.0 which can result in database corruption when database migrations are interrupted on sqlite. ([\#14926](matrix-org/synapse#14926))
- Fix a bug introduced in Synapse 1.68.0 where we were unable to service remote joins in rooms with `@room` notification levels set to `null` in their (malformed) power levels. ([\#14942](matrix-org/synapse#14942))
- Fix a bug introduced in Synapse 1.64.0 where boolean power levels were erroneously permitted in [v10 rooms](https://spec.matrix.org/v1.5/rooms/v10/). ([\#14944](matrix-org/synapse#14944))
- Fix a long-standing bug where sending messages on servers with presence enabled would spam "Re-starting finished log context" log lines. ([\#14947](matrix-org/synapse#14947))
- Fix a bug introduced in Synapse 1.68.0 where logging from the Rust module was not properly logged. ([\#14976](matrix-org/synapse#14976))
- Fix various long-standing bugs in Synapse's config, event and request handling where booleans were unintentionally accepted where an integer was expected. ([\#14945](matrix-org/synapse#14945))


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

- Add missing type hints. ([\#14879](matrix-org/synapse#14879), [\#14886](matrix-org/synapse#14886), [\#14887](matrix-org/synapse#14887), [\#14904](matrix-org/synapse#14904), [\#14927](matrix-org/synapse#14927), [\#14956](matrix-org/synapse#14956), [\#14983](matrix-org/synapse#14983), [\#14984](matrix-org/synapse#14984), [\#14985](matrix-org/synapse#14985), [\#14987](matrix-org/synapse#14987), [\#14988](matrix-org/synapse#14988), [\#14990](matrix-org/synapse#14990), [\#14991](matrix-org/synapse#14991), [\#14992](matrix-org/synapse#14992), [\#15007](matrix-org/synapse#15007))
- Use `StrCollection` to avoid potential bugs with `Collection[str]`. ([\#14922](matrix-org/synapse#14922))
- Allow running the complement tests suites with the asyncio reactor enabled. ([\#14858](matrix-org/synapse#14858))
- Improve performance of `/sync` in a few situations. ([\#14908](matrix-org/synapse#14908), [\#14970](matrix-org/synapse#14970))
- Document how to handle Dependabot pull requests. ([\#14916](matrix-org/synapse#14916))
- Fix typo in release script. ([\#14920](matrix-org/synapse#14920))
- Update build system requirements to allow building with poetry-core 1.5.0. ([\#14949](matrix-org/synapse#14949), [\#15019](matrix-org/synapse#15019))
- Add an [lnav](https://lnav.org) config file for Synapse logs to `/contrib/lnav`. ([\#14953](matrix-org/synapse#14953))
- Faster joins: Refactor internal handling of servers in room to never store an empty list. ([\#14954](matrix-org/synapse#14954))
- Faster joins: tag `v2/send_join/` requests to indicate if they served a partial join response. ([\#14950](matrix-org/synapse#14950))
- Allow running `cargo` without the `extension-module` option. ([\#14965](matrix-org/synapse#14965))
- Preparatory work for adding a denormalised event stream ordering column in the future. Contributed by Nick @ Beeper (@Fizzadar). ([\#14979](matrix-org/synapse#14979), [9cd7610](matrix-org/synapse@9cd7610), [f10caa7](matrix-org/synapse@f10caa7); see [\#15014](matrix-org/synapse#15014))
- Add tests for `_flatten_dict`. ([\#14981](matrix-org/synapse#14981), [\#15002](matrix-org/synapse#15002))

<details><summary>Locked dependency updates</summary>

- Bump dtolnay/rust-toolchain from e645b0cf01249a964ec099494d38d2da0f0b349f to 9cd00a88a73addc8617065438eff914dd08d0955. ([\#14968](matrix-org/synapse#14968))
- Bump docker/build-push-action from 3 to 4. ([\#14952](matrix-org/synapse#14952))
- Bump ijson from 3.1.4 to 3.2.0.post0. ([\#14935](matrix-org/synapse#14935))
- Bump types-pyyaml from 6.0.12.2 to 6.0.12.3. ([\#14936](matrix-org/synapse#14936))
- Bump types-jsonschema from 4.17.0.2 to 4.17.0.3. ([\#14937](matrix-org/synapse#14937))
- Bump types-pillow from 9.4.0.3 to 9.4.0.5. ([\#14938](matrix-org/synapse#14938))
- Bump hiredis from 2.0.0 to 2.1.1. ([\#14939](matrix-org/synapse#14939))
- Bump hiredis from 2.1.1 to 2.2.1. ([\#14993](matrix-org/synapse#14993))
- Bump types-setuptools from 65.6.0.3 to 67.1.0.0. ([\#14994](matrix-org/synapse#14994))
- Bump prometheus-client from 0.15.0 to 0.16.0. ([\#14995](matrix-org/synapse#14995))
- Bump anyhow from 1.0.68 to 1.0.69. ([\#14996](matrix-org/synapse#14996))
- Bump serde_json from 1.0.91 to 1.0.92. ([\#14997](matrix-org/synapse#14997))
- Bump isort from 5.11.4 to 5.11.5. ([\#14998](matrix-org/synapse#14998))
- Bump phonenumbers from 8.13.4 to 8.13.5. ([\#14999](matrix-org/synapse#14999))
</details>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@clokep clokep clokep 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
@squahtx @clokep