-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
1.x: fix reset() shutting down everything other than the schedulers #3996
Conversation
/** | ||
* Start the instance-specific schedulers. | ||
*/ | ||
synchronized void startInstance() { |
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.
Any reason to not make these private?
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.
We try to avoid too much private
modifiers; generally it adds hidden bridge methods, increasing the method count which is bad for Android. Also package level access leaves it open to same-package unit testing.
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.
Fair enough, though this case wouldn't generate synthetic accessors I don't think.
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.
Right, bridges are not in play here; it has become a second nature of me scoping as package-private.
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.
Not a bad approach :). LGTM then
Minor nit, rest of the code looks good 👍 Nice catch, sorry for the inadvertent bug and glad it was a relatively simple fix |
👍 |
1 similar comment
👍 |
The reset shut down all the main schedulers and the helper pools, causing the failure in #3993 .
@hzsweers, you were right with the need for the instance-shutdown method.