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 1 commit
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