-
Notifications
You must be signed in to change notification settings - Fork 120
fsm: add WaitForStateAsync to the cached observer #741
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
Conversation
161c4a5
to
e5b2f2b
Compare
e5b2f2b
to
e6e596b
Compare
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.
LGTM!
fsm/observer.go
Outdated
// receive an error if the expected state is reached or an error occurred. If | ||
// the context is canceled before the expected state is reached, the channel | ||
// will receive an ErrWaitingForStateTimeout error. | ||
func (s *CachedObserver) WaitForStateAsync(ctx context.Context, state StateType, |
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.
nit: rename receiver c
here and in WaitForState
.
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.
done
e6e596b
to
eddb8dc
Compare
eddb8dc
to
24c30ee
Compare
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.
tACK, tested these changes to the fsm with the static address withdrawal flow without issues.
Thanks for the explanation of the deadlock scenario.
fsm/observer.go
Outdated
} | ||
} | ||
|
||
// Otherwise, wait for the next notification | ||
s.notificationCond.Wait() | ||
// Otherwise use the conditonal variable to wait for |
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.
typo conditional
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.
done
By adding WaitForStateAsync to the observer we can always observe state changes in an atomic way without relying on the observer's internal cache.
336d336
to
811e9df
Compare
By adding WaitForStateAsync to the observer we can always observe state changes in an atomic way without relying on the observer's internal cache.
Pull Request Checklist
release_notes.md
if your PR contains major features, breaking changes or bugfixes