-
Notifications
You must be signed in to change notification settings - Fork 955
Fix flaky test_same_subnet_unsubscription() #8609
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
base: stable
Are you sure you want to change the base?
Conversation
ackintosh
left a comment
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.
Thanks, @figtracer ! I've left a few comments.
| let _ = get_events_until_num_slots(&mut subnet_service, None, 1).await; | ||
| // If permanently subscribed, no Subscribe/Unsubscribe events will be generated | ||
| if subnet_service.is_subscribed_permanent(&subnet) { | ||
| let _ = get_events_until_num_slots(&mut subnet_service, None, 3).await; |
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.
To make the test more robust, I think we should check that the event returned from get_events_until_num_slots is empty.
| // Wait for exactly 2 events (Subscribe + Unsubscribe) with a generous timeout | ||
| let expected = [ | ||
| SubnetServiceMessage::Subscribe(subnet), | ||
| SubnetServiceMessage::Unsubscribe(subnet), | ||
| ]; | ||
| let events = get_events_until_num_slots( | ||
| &mut subnet_service, | ||
| Some(2), | ||
| (MainnetEthSpec::slots_per_epoch()) as u32, | ||
| ) | ||
| .await; | ||
| assert_eq!(events, expected); |
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.
The original intent here (before this fix) was to check which event we get at each slot.
So I think it would be preferable to check the events slot by slot, for example like this:
let subscribe_event = get_events_until_num_slots(&mut subnet_service, Some(1), 1).await;
assert_eq!(subscribe_event, expected);
let unsubscribe_event = get_events_until_num_slots(&mut subnet_service, Some(1), 1).await;
assert_eq!(unsubscribe_event, expected);
Issue Addressed
#7573
Why
the race condition seems due to the usage of
is_subscribed()instead ofis_subscribed_permanent(), which was not following the comment's desired behaviour sinceis_subscribed()considers both permanent and temporary subscriptions.// If we are permanently subscribed to this subnet, we won't see a subscribe messageso, previously, we could be taking the
ifbranch for a temporary subscriptiondiscarding
Subscribeand potentiallyUnsubscribeevents; thus, when reachingthe expected
Unsubscribecould have been discarded already, resulting in [].Fix
refactored the test to use a more deterministic approach following the other tests and removed
is_subscribed()since we take the same route by checkingis_subscribed_permanent()firstResults