Add more tests for receiving events while resyncing for a partial join #419
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ import ( | |
| "github.com/gorilla/mux" | ||
| "github.com/tidwall/gjson" | ||
|
|
||
| "github.com/matrix-org/gomatrix" | ||
| "github.com/matrix-org/gomatrixserverlib" | ||
| "github.com/matrix-org/util" | ||
|
|
||
|
|
@@ -489,6 +490,51 @@ func testReceiveEventDuringPartialStateJoin( | |
| t.Fatalf("GET /event failed with %d: %s", res.StatusCode, string(eventResBody)) | ||
| } | ||
|
|
||
| // fire off a /state_ids request for the last event. | ||
| // it must either: | ||
| // * block because the homeserver does not have full state at the last event | ||
| // * or 403 because the homeserver does not have full state yet and does not consider the | ||
| // Complement homeserver to be in the room | ||
| stateIdsResponseChan := make(chan *gomatrixserverlib.RespStateIDs) | ||
| defer close(stateIdsResponseChan) | ||
| go func() { | ||
| stateReq := gomatrixserverlib.NewFederationRequest("GET", "hs1", | ||
| fmt.Sprintf("/_matrix/federation/v1/state_ids/%s?event_id=%s", | ||
| url.PathEscape(psjResult.ServerRoom.RoomID), | ||
| url.QueryEscape(event.EventID()), | ||
| ), | ||
| ) | ||
| var respStateIDs gomatrixserverlib.RespStateIDs | ||
| if err := psjResult.Server.SendFederationRequest(deployment, stateReq, &respStateIDs); err != nil { | ||
| httpErr, ok := err.(gomatrix.HTTPError) | ||
| t.Logf("%v", httpErr) | ||
| if ok && httpErr.Code == 403 { | ||
| stateIdsResponseChan <- nil | ||
| return | ||
| } | ||
| t.Errorf("/state_ids request returned non-200: %s", err) | ||
| return | ||
| } | ||
| stateIdsResponseChan <- &respStateIDs | ||
|
There was a problem hiding this comment. rather than matching for the 403 here and sending a type stateIDsResult struct {
RespStateIDs gomatrixserverlib.RespStateIDs
Error error
}... and then send that back down the channel, and do the matching in the main thread. |
||
| }() | ||
|
|
||
| select { | ||
| case <-time.After(1 * time.Second): | ||
| t.Logf("/state_ids request for event %s blocked as expected", event.EventID()) | ||
| defer func() { <-stateIdsResponseChan }() | ||
|
There was a problem hiding this comment. when the There was a problem hiding this comment. right, so we're making sure we read from the channel so that the goroutine doesn't block. Can you add a comment to that effect? |
||
| break | ||
| case respStateIDs := <-stateIdsResponseChan: | ||
| if respStateIDs == nil { | ||
| t.Logf("/state_ids request for event %s returned 403 as expected", event.EventID()) | ||
| } else { | ||
| // since we have not yet given the homeserver the full state at the join event and allowed | ||
| // the partial join to complete, it can't possibly know the full state at the last event. | ||
| // While it may be possible for the response to be correct by some accident of state res, | ||
| // the homeserver is still wrong in spirit. | ||
| t.Fatalf("/state_ids request for event %s did not block when it should have", event.EventID()) | ||
|
There was a problem hiding this comment. This part tests matrix-org/synapse#13002 . If Synapse does not correctly flag that it does not have partial state in the |
||
| } | ||
| } | ||
|
|
||
| // allow the partial join to complete | ||
| psjResult.FinishStateRequest() | ||
| alice.MustSyncUntil(t, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we know which, and test for that specifically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now only the last test case blocks instead of 403s. I'm somewhat reluctant to be more specific in the tests, because it feels like an implementation detail of synapse (which will change when we get around to matrix-org/synapse#13288).
If you're really keen on being more specific, I'm happy to add a boolean parameter or suchlike. (It's not easy to move this part of the test out, since it needs to happen after the /event request above has succeeded)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah right. tbh I don't think either blocking or returning a 403 is correct behaviour, though I'm not quite sure what is.
It would be very useful to add a link to #13288 and say that this is somewhat temporary until we solve it properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, is there a better way to test whether synapse thinks an event has partial state?
this part of the test was intended to check that
_resolve_state_at_missing_prevscalculates the partial state flag correctly and it sounds like it's not a good idea to rely on /state_idsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
none that I can immediately think of. The ideal thing would be to check that after the resync, /state_ids returns some result that would have been impossible without the information conveyed during the resync - but that sounds rather fiddly for a test (and, probably, a hard-to-understand test failure if it later turns out not to be true).