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

Hystrix.reset() now shuts down the HystrixTimer #111

Merged
merged 1 commit into from
Feb 22, 2013
Merged

Hystrix.reset() now shuts down the HystrixTimer #111

merged 1 commit into from
Feb 22, 2013

Conversation

benjchristensen
Copy link
Contributor

  • the HystrixTimer can be shutdown via an interrupt (which Hystrix.reset uses)
  • the HystrixTimer will correctly restart the thread if it is used again after a reset

#109

- the HystrixTimer can be shutdown via an interrupt (which Hystrix.reset uses)
- the HystrixTimer will correctly restart the thread if it is used again after a reset

#109
@daveray
Copy link
Contributor

daveray commented Feb 22, 2013

Looks reasonable to me. The listeners map and intervals queue seem like they're coordinated so you could have race conditions between the reset code and listener registration. I could also imagine pathological TimerListener.tick() code that swallows/suppresses interrupts which would cause shutdown not to occur. But, honestly, this code should only be executed during a controlled shutdown so it's probably fine.

@benjchristensen
Copy link
Contributor Author

Yes, if someone tries to reset while still using Hystrix then it won't be thread-safe and race conditions can occur. I comment on that in the javadoc:

NOTE: This will result in race conditions if {@link #addTimerListener(com.netflix.hystrix.util.HystrixTimer.TimerListener)} is being concurrently called.

And yes, if the tick() implementation was evil it could swallow the interrupt - good point, I hadn't considered that. We are the only ones who create tick() implementations so that should be fine - but I could instead use a separate variable to ask for it to stop rather than using interrupt.

benjchristensen added a commit that referenced this pull request Feb 22, 2013
Hystrix.reset() now shuts down the HystrixTimer
@benjchristensen benjchristensen merged commit 91244ed into Netflix:master Feb 22, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants