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

Fix losing incoming EDUs if debug logging enabled #11890

Merged
merged 3 commits into from
Feb 2, 2022
Merged

Fix losing incoming EDUs if debug logging enabled #11890

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
56ac450
Fix losing incoming EDUs if debug logging enabled
Feb 2, 2022
e2c320a
Changelog
Feb 2, 2022
5130547
Don't expect the "edu_type" to be hashable
Feb 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/11890.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a bug introduced in Synapse 1.51.0rc1 where incoming federation transactions containing at least one EDU would be dropped if debug logging was enabled for `synapse.8631_debug`.
4 changes: 2 additions & 2 deletions synapse/federation/transport/server/federation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,11 +109,11 @@ async def on_PUT(
)

if issue_8631_logger.isEnabledFor(logging.DEBUG):
DEVICE_UPDATE_EDUS = {"m.device_list_update", "m.signing_key_update"}
DEVICE_UPDATE_EDUS = ["m.device_list_update", "m.signing_key_update"]
device_list_updates = [
edu.content
Copy link
Contributor Author

Choose a reason for hiding this comment

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

edu.content does not exist.

for edu in transaction_data.get("edus", [])
if edu.edu_type in DEVICE_UPDATE_EDUS
if edu.get("edu_type") in DEVICE_UPDATE_EDUS
Copy link
Member

Choose a reason for hiding this comment

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

This could still error if the edu_type is weird (e.g. a list). A way around this is making DEVICE_UPDATE_EDUS into a list, not a set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see, I'm implicitly assuming that the edu_type is hashable.

Copy link
Member

Choose a reason for hiding this comment

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

Yep! It is probably OK, but if the downside is breaking federation...that's pretty bad.

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 done. I haven't fixed up the TX side here

synapse/synapse/federation/sender/transaction_manager.py

Lines 128 to 138 in f160fe1

if issue_8631_logger.isEnabledFor(logging.DEBUG):
DEVICE_UPDATE_EDUS = {"m.device_list_update", "m.signing_key_update"}
device_list_updates = [
edu.content for edu in edus if edu.edu_type in DEVICE_UPDATE_EDUS
]
if device_list_updates:
issue_8631_logger.debug(
"about to send txn [%s] including device list updates: %s",
transaction.transaction_id,
device_list_updates,
)

because there we really do have an Edu struct, and its type is something we should be fully in control of.

]
if device_list_updates:
issue_8631_logger.debug(
Expand Down