Make request_allowed? thread safe

[casperisfine]

First draft for #86

The idea here is to remove any side effect request_allowed? might have, so that it's safe to call it from a threaded environment.

NB: The other circuit breaker methods stays unsafe and it's the adapter's responsibility to call them safely.

[dylanahsmith]

error_timeout_expired? uses @errors.last which is calling .last on an array. That is probably safe to do in ruby due to the vm lock, but we might want to have a @last instance variable in the SlidingWindow class if you want it to be accessed in a thread-safe way for jruby.

[casperisfine]

Indeed. But JRuby isn't really a concern at this point since we rely on C extensions.

[eapache]

Might want to leave a comment that open? && error_timeout_expired? is intended as a non-mutating equivalent check to "should transition to half-open"?

Or should that condition even just go in half_open? to begin with?

[casperisfine]

Or should that condition even just go in half_open? to begin with?

It can't really go in half_open? because then you wouldn't know if you have to make a transition or not. So yeah that piece isn't necessarily trivial to understand. I'm usually not a comment guy but it's likely worth it here.

[dylanahsmith]

Seems like the right approach.

It just looks like CI isn't setup correctly anymore, since it is getting connect errors and retrying a CI run on master produced errors now when it was passing 29 days ago.

[casperisfine]

Ok, CI is green now (thx Dylan).

@dylanahsmith @csfrancis @sirupsen @eapache mind reviewing?

[dylanahsmith]

👍

[eapache]

👍

[sirupsen]

👍

[dylanahsmith]

This looks like it could be tested by using request_allowed? in a circuit breaker test and asserting that it doesn't change the state when the error timeout expires.

[dylanahsmith]

It looks like only UnprotectedResource#request_allowed? is tested in the test suite. It doesn't look like there are any tests for CircuitBreaker#request_allowed? or ProtectedResource#request_allowed? which delegates to the circuit breaker.

[casperisfine]

I added a test, but since the state query methods were all private I had to publicize them.

And yes CircuitBreaker is mostly tested in an integration way.

[dylanahsmith]

I guess thread safety is hard to test in an integration way. Thanks for adding the test though

[casperisfine]

Released as 0.5.2