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

Actually fix bad debug logging rejecting device list & signing key transactions #12098

Merged
merged 7 commits into from
Feb 28, 2022
Merged

Actually fix bad debug logging rejecting device list & signing key transactions #12098

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e21a6d2
Actually fix bad logging rejecting incoming transactions
Feb 28, 2022
2e36f23
Changelog
Feb 28, 2022
39022bd
Test case
Feb 28, 2022
6b0446a
Merge branch 'develop' into dmr/fix-edu-debugging-again
Feb 28, 2022
4f4127b
Don't use RX
Feb 28, 2022
1cfd4e5
Simpler changelog
Feb 28, 2022
d950e3b
Linter script
Feb 28, 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/12098.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`. Synapse 1.53.0rc1 included a partial fix this ([#11890](https://github.com/matrix-org/synapse/pull/11890)), but the fix was incorrect: EDUs containing device list or signing key updates would still be dropped.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure it's worth mentioning #11890 since it didn't end up fixing 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.

I just want to clarify that I've not accidentally copied and pasted a bugfix line from 1.53's release notes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough, though I'd tend to think looking at the PR is enough to tell that

2 changes: 1 addition & 1 deletion synapse/federation/transport/server/federation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ async def on_PUT(
if issue_8631_logger.isEnabledFor(logging.DEBUG):
DEVICE_UPDATE_EDUS = ["m.device_list_update", "m.signing_key_update"]
device_list_updates = [
edu.content
edu.get("content", {})
for edu in transaction_data.get("edus", [])
if edu.get("edu_type") in DEVICE_UPDATE_EDUS
]
Expand Down
18 changes: 17 additions & 1 deletion tests/federation/transport/test_server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
# limitations under the License.

from tests import unittest
from tests.unittest import override_config
from tests.unittest import override_config, DEBUG


class RoomDirectoryFederationTests(unittest.FederatingHomeserverTestCase):
Expand All @@ -38,3 +38,19 @@ def test_open_public_room_list_over_federation(self):
"/_matrix/federation/v1/publicRooms",
)
self.assertEqual(200, channel.code)

@DEBUG
def test_edu_debugging_doesnt_explode(self):
"""Sanity check fed. RX succeeds with `synapse.debug_8631` logging enabled.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what "RX" means, could you update this comment please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TX: transmit
RX: receive

not sure where the abbreviation comes from. Synapse uses the former in transaction_manager.py and per_destination_queue.py. Slightly surprised to see it doesn't use the latter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possible etymology: https://en.wikipedia.org/wiki/RS-232

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! fwiw I just read TX in those file as an abbreviated version of txn for "transaction", TIL TX and RX mean transmit and receive.


Remove this when we strip out issue_8631_logger.
"""
channel = self.make_signed_federation_request(
"PUT",
"/_matrix/federation/v1/send/txn_id_1234/",
content={
"edus": [{"edu_type": "m.device_list_update", "content": {"foo": "bar"}}],
"pdus": [],
},
)
self.assertEqual(200, channel.code)