bolshakov / stoplight

:traffic_light: Traffic control for code.
http://bolshakov.github.io/stoplight/
MIT License
384 stars 40 forks source link

Clarify using timeout to disable automatic recovery #73

Closed tfe closed 9 years ago

tfe commented 9 years ago

In the readme, it says: "Set the timeout to -1 to disable automatic recovery." Based on this, I would have expected that after enough failures it would remain red indefinitely (i.e. not automatically recover), but this is not the case. It actually goes immediately back to yellow (because it's been longer since the failure than the negative timeout).

Is this a bug or am I just reading it incorrectly?

tfausak commented 9 years ago

Thanks for reporting this! It does indeed look like a bug.

light = Stoplight('example') { fail }.with_timeout(-1)
light.threshold.times { light.run rescue nil }
light.color
# => "yellow"

I think the light should be in the red state at that point, but I'm having a hard time finding a commit where that's the case. Only one line reads the timeout, and it doesn't check for negative values.

We didn't introduce timeouts in a pull request, so I don't have any discussion to reference. However #37 looks relevant.

tfausak commented 9 years ago

As a workaround, you can set the timeout to Float::INFINITY.

tfausak commented 9 years ago

I think the documentation is inaccurate and should be changed. Instead of recommending -1 for disabling automatic recovery, it should recommend Float::INFINITY. That requires no change to the code and is more intention-revealing at the call site. Consider the following examples:

# What does this mean? I'd have to read the documentation to find out.
Stoplight('...') { ... }.with_timeout(-1)
# This is pretty obvious.
Stoplight('...') { ... }.with_timeout(Float::INFINITY)

Does this solution work for you?

tfe commented 9 years ago

That works for me! Totally agree on the intention-revealingness aspect.