Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Delete empty dm room on createRoom error #1912

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: Delete empty dm room on createRoom error #1912

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
56 changes: 39 additions & 17 deletions lib/src/client.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -795,27 +795,49 @@ class Client extends MatrixApi {
));
}
}
try {
// Start a new direct chat
final roomId = await createRoom(
invite: [mxid],
isDirect: true,
preset: preset,
initialState: initialState,
powerLevelContentOverride: powerLevelContentOverride,
);

// Start a new direct chat
final roomId = await createRoom(
invite: [mxid],
isDirect: true,
preset: preset,
initialState: initialState,
powerLevelContentOverride: powerLevelContentOverride,
);

if (waitForSync) {
final room = getRoomById(roomId);
if (room == null || room.membership != Membership.join) {
// Wait for room actually appears in sync
await waitForRoomInSync(roomId, join: true);
if (waitForSync) {
final room = getRoomById(roomId);
if (room == null || room.membership != Membership.join) {
// Wait for room actually appears in sync
await waitForRoomInSync(roomId, join: true);
}
}
}

await Room(id: roomId, client: this).addToDirectChat(mxid);
await Room(id: roomId, client: this).addToDirectChat(mxid);

return roomId;
return roomId;
} catch (e) {
Copy link
Member

Choose a reason for hiding this comment

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

This is catching any error, not just the room creation error. You should narrowly scope the try catch and handle specific errors, not all of them and assume it is the one error you intended to handle.

/// This looks up the room id of the room that was just created.
/// The room is empty since createRoom threw an error. To make sure not to
/// delete other empty rooms by accident, the creation time is checked.
/// Should be fixed in Synapse and then be removed
Comment on lines +821 to +823
Copy link
Member

Choose a reason for hiding this comment

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

You should really document, why this throws an error, because the synapse code tries to handle all errors ahead of time, so it is not clear, why you are getting an error on room creation. If the problem is invites, we can for example move those out of the room creation.

final sortedRooms = rooms.sortedBy((room) => room.timeCreated).reversed;
Copy link
Member

Choose a reason for hiding this comment

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

How do you know, that the room is already synced back to the client?

final emptyRoom = sortedRooms.firstWhereOrNull((room) =>
room.name.isEmpty &&
!room.isDirectChat &&
room.summary.mJoinedMemberCount == 1 &&
room.summary.mInvitedMemberCount == 0 &&
room.timeCreated
.isAfter(DateTime.now().subtract(Duration(seconds: 20))));
final roomId = emptyRoom?.id;
Comment on lines +824 to +832
Copy link
Member

Choose a reason for hiding this comment

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

it's probably more robust to handle the room id list differences from before and after the room creation instead of going purely by timestamp?


/// If we can find an empty unnamed room that was recently created, we leave and delete it
if (roomId != null) {
await leaveRoom(roomId);
await forgetRoom(roomId);
Comment on lines +836 to +837
Copy link
Member

Choose a reason for hiding this comment

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

This can throw and needs error handling to not cause weird error messages.

}
rethrow;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This code needs tests imo.


/// Simplified method to create a new group chat. By default it is a private
Expand Down
Loading