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

_run_push_actions_and_persist_event: handle no min_depth #11014

Merged
merged 2 commits into from
Oct 18, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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/11014.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add some extra logging to the event persistence code.
28 changes: 18 additions & 10 deletions synapse/handlers/federation_event.py
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ async def on_receive_pdu(self, origin: str, pdu: EventBase) -> None:

if missing_prevs:
# We only backfill backwards to the min depth.
min_depth = await self.get_min_depth_for_context(pdu.room_id)
min_depth = await self._store.get_min_depth(pdu.room_id)
logger.debug("min_depth: %d", min_depth)

if min_depth is not None and pdu.depth > min_depth:
Expand Down Expand Up @@ -1690,16 +1690,27 @@ async def _run_push_actions_and_persist_event(
# persist_events_and_notify directly.)
assert not event.internal_metadata.outlier

try:
if (
not backfilled
and not context.rejected
and (await self._store.get_min_depth(event.room_id)) <= event.depth
):
if not backfilled and not context.rejected:
min_depth = await self._store.get_min_depth(event.room_id)
if min_depth is None or min_depth > event.depth:
# XXX richvdh 2021/10/07: I don't really understand what this
# condition is doing. I think it's trying not to send pushes
# for events that predate our join - but that's not really what
# min_depth means, and anyway ancient events are a more general
# problem.
#
# for now I'm just going to log about it.
Comment on lines +1696 to +1702
Copy link
Contributor

Choose a reason for hiding this comment

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

For context, this condition was added in #10614 (though the PR doesn't explain what it's is doing)

Copy link
Member Author

Choose a reason for hiding this comment

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

no, #10614 just moved the logic from https://github.com/matrix-org/synapse/pull/10614/files#diff-0bc92da3d703202f5b9be2d3f845e375f5b1a6bc6ba61705a8af9be1121f5e42L296. Still, thanks for the link, since it shows a comment I'd deleted 🤦 .

Anyway, the comment sort-of confirms my thinking about what this is trying to do - but I still think it's going about it wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

no, #10614 just moved the logic from https://github.com/matrix-org/synapse/pull/10614/files#diff-0bc92da3d703202f5b9be2d3f845e375f5b1a6bc6ba61705a8af9be1121f5e42L296. Still, thanks for the link, since it shows a comment I'd deleted 🤦.

Oh, indeed, I misread the diff.

logger.info(
"Skipping push actions for old event with depth %s < %s",
event.depth,
min_depth,
)
else:
await self._action_generator.handle_push_actions_for_event(
event, context
)

try:
await self.persist_events_and_notify(
event.room_id, [(event, context)], backfilled=backfilled
)
Expand Down Expand Up @@ -1831,6 +1842,3 @@ def _sanity_check_event(self, ev: EventBase) -> None:
len(ev.auth_event_ids()),
)
raise SynapseError(HTTPStatus.BAD_REQUEST, "Too many auth_events")

async def get_min_depth_for_context(self, context: str) -> int:
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've just inlined this, since it was a bit pointless.

return await self._store.get_min_depth(context)
2 changes: 1 addition & 1 deletion synapse/storage/databases/main/event_federation.py
Original file line number Diff line number Diff line change
Expand Up @@ -906,7 +906,7 @@ async def get_latest_event_ids_in_room(self, room_id: str) -> List[str]:
desc="get_latest_event_ids_in_room",
)

async def get_min_depth(self, room_id: str) -> int:
async def get_min_depth(self, room_id: str) -> Optional[int]:
Copy link
Member Author

Choose a reason for hiding this comment

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

this is a fix to the type annotation.

"""For the given room, get the minimum depth we have seen for it."""
return await self.db_pool.runInteraction(
"get_min_depth", self._get_min_depth_interaction, room_id
Expand Down