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.

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

Have the room member tests retry a few times #631

Merged
merged 6 commits into from Jul 1, 2019

Conversation

3 participants
@anoadragon453
Copy link
Member

commented Jun 26, 2019

These tests were pretty flaky in a multi-process homeserver (like Dendrite). The events took a few moments to persist, and thus a race condition was produced where SyTest would request the status right afterwards (before the events finished persisting).

We now retry a few times before calling it failed.

(Should retry_until_success be moved inside of matrix_get_room_state?)

@anoadragon453 anoadragon453 requested a review from matrix-org/synapse-core Jun 26, 2019

@anoadragon453 anoadragon453 added this to In progress in Homeserver Task Board via automation Jun 26, 2019

@richvdh
Copy link
Member

left a comment

I'm not quite sure what I think about this. Tightlooping on get_room_state doesn't seem optimal, though I guess it's ok.

In other places I think we wait for the change to come down /sync rather than loop on get_room_state. @erikjohnston do you have any thoughts on this?

type => "m.room.member",
state_key => $user->user_id,
)
}
})->then( sub {

This comment has been minimized.

Copy link
@richvdh

richvdh Jun 26, 2019

Member

should this not be inside the retry_until_success call?

This comment has been minimized.

Copy link
@richvdh

richvdh Jun 26, 2019

Member

(likewise various other places below)

This comment has been minimized.

Copy link
@anoadragon453

anoadragon453 Jun 26, 2019

Author Member

The whole subroutine?

This comment has been minimized.

Copy link
@richvdh

richvdh Jun 27, 2019

Member

I meant the bit where you check the body of the state event is what you expect it to be, but I've now realised that it's bogus since matrix_get_room_state will return a 404 until the join completes, so it's fine in this case. Still, there are other places where you are checking that the membership has turned from join to ban, or the PLs have changed, that look wrong to me.

Show resolved Hide resolved tests/10apidoc/33room-members.pl Outdated
@erikjohnston

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

I'm not quite sure what I think about this. Tightlooping on get_room_state doesn't seem optimal, though I guess it's ok.

FWIW it does have a backoff delay, so it doesn't hammer tooooo much

In other places I think we wait for the change to come down /sync

We do tend to use *_synced as they're just more convenient, though there's nothing actually that says if the event has made it to the sync servers its made it to the necessary other workers....

type => "m.room.member",
state_key => $user->user_id,
)
}
})->then( sub {

This comment has been minimized.

Copy link
@richvdh

richvdh Jun 27, 2019

Member

I meant the bit where you check the body of the state event is what you expect it to be, but I've now realised that it's bogus since matrix_get_room_state will return a 404 until the join completes, so it's fine in this case. Still, there are other places where you are checking that the membership has turned from join to ban, or the PLs have changed, that look wrong to me.

@anoadragon453

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2019

Fixed up with advice from rich in #synapse-dev.

@richvdh
Copy link
Member

left a comment

nearly...

matrix_get_room_state( $user, $room_id, type => "m.room.power_levels" )
retry_until_success {
matrix_get_room_state( $user, $room_id, type => "m.room.power_levels" )
}
})->then( sub {

This comment has been minimized.

Copy link
@richvdh

richvdh Jul 1, 2019

Member

needs to be in the retry_until_success block

@richvdh

richvdh approved these changes Jul 1, 2019

Copy link
Member

left a comment

lgtm

@anoadragon453 anoadragon453 merged commit 47ccc3f into develop Jul 1, 2019

Homeserver Task Board automation moved this from In progress to Done Jul 1, 2019

@anoadragon453 anoadragon453 deleted the anoa/room_members_less_flake branch Jul 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.