Retry /publicRooms a few times to allow for propagation delay before a new room appears in the room directory #415
Conversation
reivilibre
force-pushed
the
rei/publicrooms_propagation
branch
2 times, most recently
from
535257b
to
816cfa3
Compare
There was a problem hiding this comment.
Drive-by thoughts. No idea what a Waiter is (https://pkg.go.dev/github.com/aws/aws-sdk-go/private/waiter#Waiter?) but if it's more idiomatic then that might be the way to go.
There was a problem hiding this comment.
What we want is something which will repeat the action until some condition is satisfied, at which point it will return the response. This response can then have assertions applied to it. Currently, this PR conflates the two: the presence of the room ID is both the condition to satisfy and the assertion. This doesn't compose well because:
- You cannot use assertion helpers (like matchers) or anything which will fail the test as part of your code to repeat until the condition is satisfied, as the first failure will fail the test.
- You cannot say things like "do this request until you see this field, then give me the response so I can inspect the fields more deeply".
| authedClient.MustDoFuncUntil(t, "GET", []string{"_matrix", "client", "v3", "publicRooms"}, func(res *http.Response) bool { | ||
| foundRoom := false | ||
|
|
||
| must.MatchResponse(t, res, match.HTTPResponse{ |
There was a problem hiding this comment.
You can't use MatchResponse in this way as it will fail the test if the response does not pass the given matchers.
There was a problem hiding this comment.
(NB Diff is outdated but basic structure remains the same, so will reply to comment)
I believe this is what we want, though (or at least I'm used to it from SyTest); if the response is ever malformed then we want to fail the test immediately without waiting for it to be correct.
Put another way: The retries are waiting for the expected data to appear in the response, but that doesn't mean that any responses before that are allowed to omit required fields.
As a result, you end up with 3 outcomes: success, temporary fail (i.e. retry) and permanent fail.
As for how these currently seem to map to code:
Success = return true
Temporary fail = return false
Permanent fail = t.FailNow/t.Fatalf
This will repeat a request until the condition is satisfied. Intended use: #415
reivilibre
force-pushed
the
rei/publicrooms_propagation
branch
from
40a4116
to
5eb18d2
Compare
There was a problem hiding this comment.
Seems legit to me. Will defer to @kegsay that this is the Right Way
| foundRoom = true | ||
| return nil | ||
| } | ||
| return nil |
There was a problem hiding this comment.
This is a slight misuse of matchers, given you always return nil it will always succeed even if the room isn't found, and then you side-effect foundRoom to determine retry-ness. It's not something that the matchers were designed for, but in practice it seems to read pretty well.
There was a problem hiding this comment.
You're right, though I will say 'don't blame me, it was there before!' :-P
I suspect this sort of pattern will crop up again, so if you get any better ideas then it may be worth putting them in
This is needed in Synapse because the room directory is updated in the background. Part of matrix-org/synapse#13161.
It's particularly worse when using workers, because the room directory is updated in a different process and CPU contention increases the likelihood of hitting the race.