-
Notifications
You must be signed in to change notification settings - Fork 10
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
Ensure polling thread is resilient to errors and exceptions #60
Ensure polling thread is resilient to errors and exceptions #60
Conversation
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 @goncalossilva, this looks great. I've added one minor comment requesting another test case but otherwise, all good from me.
82b3e5c
to
022a9fd
Compare
@matthewelwell Thanks for the quick review! Added handling for |
022a9fd
to
0a4768c
Compare
Co-authored-by: Kim Gustyr <khvn26@gmail.com>
0a3ff91
to
e494134
Compare
Co-authored-by: Kim Gustyr <khvn26@gmail.com>
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 👍
@matthewelwell @khvn26 all requested changes were applied. Should we merge this? :) |
The outstanding comment was addressed.
Flagsmith's polling thread is not resilient to API errors. If for whatever reason the API response isn't a 200,
FlagsmithAPIError
is thrown and never caught, leaving the SDK running with a dead a polling thread and consequently stale data, even if the source of the error is addressed in the meantime.Note: There are many ways to bake resiliency into this. This made the most sense to me, but you are welcome to tweak the existing code as you see fit.