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

Jump to bottom

Test re-syncing state from other servers listed in /send_join reply #379

Merged
merged 11 commits into from
Jun 1, 2022
Merged

Test re-syncing state from other servers listed in /send_join reply #379

merged 11 commits into from
Jun 1, 2022

Conversation

squahtx
Copy link
Contributor

@squahtx squahtx commented May 25, 2022 •
edited
Loading

Sean Quah added 5 commits May 25, 2022 20:33
Set the depth of join events returned from /make_join
9726ba5 
Signed-off-by: Sean Quah <seanq@matrix.org>
Include other joined homeservers in servers_in_room /send_join re…
6729788 
…sponse

Signed-off-by: Sean Quah <seanq@matrix.org>
Add Server.Room(roomID) function
cadb31d 
Signed-off-by: Sean Quah <seanq@matrix.org>
Update AuthChainForEvents to return all auth events recursively
8a928a2 
Signed-off-by: Sean Quah <seanq@matrix.org>
Test re-syncing state from other servers listed in /send_join reply
e64fcc0 
Test that a homeserver can re-sync state from other homeservers in a
partial-state room if the server it joined off is not working.

Signed-off-by: Sean Quah <seanq@matrix.org>
squahtx
squahtx commented May 25, 2022
View reviewed changes
internal/federation/handle.go Outdated
@@ -45,6 +45,7 @@ func MakeJoinRequestsHandler(s *Server, w http.ResponseWriter, req *http.Request
Type: "m.room.member",
StateKey: &userID,
PrevEvents: []string{room.Timeline[len(room.Timeline)-1].EventID()},
Depth: room.Timeline[len(room.Timeline)-1].Depth() + 1,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a drive-by fix and doesn't affect the outcome of the test.

Copy link
Member

Choose a reason for hiding this comment

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

Please don't mix this with tests where possible. The set of reviewers for $feature are different from the set of reviewers for handling Complement internals.

@squahtx squahtx requested a review from a team May 25, 2022 19:44
@squahtx squahtx mentioned this pull request May 25, 2022
Merged
kegsay
kegsay approved these changes May 27, 2022
View reviewed changes
Copy link
Member

@kegsay kegsay left a comment

Choose a reason for hiding this comment

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

Overall this is an excellent PR! Thanks!

internal/federation/handle.go Outdated
@@ -45,6 +45,7 @@ func MakeJoinRequestsHandler(s *Server, w http.ResponseWriter, req *http.Request
Type: "m.room.member",
StateKey: &userID,
PrevEvents: []string{room.Timeline[len(room.Timeline)-1].EventID()},
Depth: room.Timeline[len(room.Timeline)-1].Depth() + 1,
Copy link
Member

Choose a reason for hiding this comment

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

Please don't mix this with tests where possible. The set of reviewers for $feature are different from the set of reviewers for handling Complement internals.

internal/federation/server.go Outdated
@@ -119,6 +119,11 @@ func NewServer(t *testing.T, deployment *docker.Deployment, opts ...func(*Server
return srv
}

// Room returns the given room
func (s *Server) Room(roomID string) *ServerRoom {
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 not required. You call MustJoinRoom in the test which returns the *ServerRoom already.

tests/federation_room_join_partial_state_test.go Outdated
defer cancelListener()

// join complement to the public room
server.MustJoinRoom(t, deployment, "hs1", roomID, server.UserID("bob"))
Copy link
Member

Choose a reason for hiding this comment

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

This returns a *ServerRoom so you don't need to then retrieve it with the new server.Room(roomID) function on :245.

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 missed that, thank you!

tests/federation_room_join_partial_state_test.go Outdated
}

// reply to hs2 with a bogus /state_ids response
sendResponseWaiter.Finish()
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 have a clearer name, e.g sendStateIDsResponse, as otherwise it's unclear which response you mean and need to add comments to explain 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'll rename them to follow the naming in partialStateJoinResult:

type partialStateJoinResult struct {
	cancelListener                   func()
	Server                           *federation.Server
	ServerRoom                       *federation.ServerRoom
	fedStateIdsRequestReceivedWaiter *Waiter
	fedStateIdsSendResponseWaiter    *Waiter
}

@squahtx squahtx requested a review from kegsay May 27, 2022 12:40
@squahtx
Copy link
Contributor Author

squahtx commented May 27, 2022

@kegsay Thanks for reviewing. Could you re-review the latest changes?

richvdh
richvdh approved these changes May 30, 2022
View reviewed changes
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm other than a few tweaks

tests/federation_room_join_partial_state_test.go Outdated
defer cancelListener()

// join complement to the public room
room := server.MustJoinRoom(t, deployment, "hs1", roomID, server.UserID("bob"))
Copy link
Member

Choose a reason for hiding this comment

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

worth noting there is also a bob@hs1 created by the blueprint. It might be better to pick a fourth username.

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 went for david instead.

tests/federation_room_join_partial_state_test.go Outdated
queryParams := req.URL.Query()
t.Logf("Incoming state_ids request for event %s in room %s", queryParams["event_id"], roomID)
fedStateIdsRequestReceivedWaiter.Finish()
fedStateIdsSendResponseWaiter.Waitf(t, 60*time.Second, "Waiting for /state request")
Copy link
Member

Choose a reason for hiding this comment

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

"Waiting for /state request" doesn't seem the right message here. I think this will only time out if the test fails elsewhere anyway, so .Wait without a dedicated message might be more appropriate.

squahtx and others added 2 commits May 31, 2022 12:46
@squahtx @richvdh
Update tests/federation_room_join_partial_state_test.go
7e104f4 
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Address PR feedback
2fdc71e 
Rename bob to david and drop the timeout message.
richvdh
richvdh approved these changes Jun 1, 2022
View reviewed changes
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm, and I need to build stuff on top of it, so I'm going to go ahead and merge it.

@richvdh richvdh merged commit caf006f into main Jun 1, 2022
@richvdh richvdh deleted the squah/faster_room_joins_resync_from_other_homeservers_on_failure branch June 1, 2022 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richvdh richvdh richvdh approved these changes

@kegsay kegsay Awaiting requested review from kegsay kegsay is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@squahtx @richvdh @kegsay