Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ Metrics/MethodLength:

Metrics/ClassLength:
Max: 500
Exclude:
- test/**/*

Metrics/AbcSize:
Max: 50
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,7 @@ There are three configuration parameters for circuit breakers in Semian:
again.
* **success_threshold**. The amount of successes on the circuit until closing it
again, that is to start accepting all requests to the circuit.
* **half_open_resource_timeout**. Timeout for the resource in seconds when the circuit is half-open (only supported for MySQL).
* **half_open_resource_timeout**. Timeout for the resource in seconds when the circuit is half-open (only supported for MySQL and Net::HTTP).

### Bulkheading

Expand Down
15 changes: 7 additions & 8 deletions lib/semian/circuit_breaker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -125,15 +125,14 @@ def disabled?
end

def maybe_with_half_open_resource_timeout(resource, &block)
result = nil

if half_open? && @half_open_resource_timeout && resource.respond_to?(:with_resource_timeout)
resource.with_resource_timeout(@half_open_resource_timeout) do
result = block.call
result =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change this? 👂

Copy link
Member Author

@max611 max611 Oct 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small bikeshed, I can bring back the old syntax. I am used to assigning the value directly from the if, not sure which syntax you guys prefer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for context: The reason this kind of thing might frowned upon is because it causes a 7 line diff that we have to review manually to ensure that a typo size bug isn't added to the library.

if half_open? && @half_open_resource_timeout && resource.respond_to?(:with_resource_timeout)
resource.with_resource_timeout(@half_open_resource_timeout) do
block.call
end
else
block.call
end
else
result = block.call
end

result
end
Expand Down
13 changes: 13 additions & 0 deletions lib/semian/net_http.rb
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,19 @@ def transport_request(*)
end
end

def with_resource_timeout(timeout)
prev_read_timeout = read_timeout
prev_open_timeout = open_timeout
begin
self.read_timeout = timeout
self.open_timeout = timeout
yield
ensure
self.read_timeout = prev_read_timeout
self.open_timeout = prev_open_timeout
end
end

private

def handle_error_responses(result)
Expand Down
42 changes: 42 additions & 0 deletions test/net_http_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,42 @@ def test_semian_identifier
end
end

def test_changes_timeout_when_half_open_and_configured
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I'm doing this right, is there a better way of half opening the circuit?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's half_open_circuit!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'll need help with this test, I can't seem to be able to trigger the right thing. I do hit the with_resource_timeout path but ReadTimeout does not raise.

http = Net::HTTP.new(SemianConfig['toxiproxy_upstream_host'], SemianConfig['http_toxiproxy_port'])
expected_read_timeout = http.read_timeout
expected_open_timeout = http.open_timeout
options = proc do |host, port|
{
tickets: 3,
success_threshold: 2,
error_threshold: 2,
error_timeout: 10,
open_circuit_server_errors: true,
name: "#{host}_#{port}",
half_open_resource_timeout: 1,
}
end

with_semian_configuration(options) do
with_server do
Toxiproxy['semian_test_net_http'].downstream(:latency, latency: 2000).apply do
http.get('/200')
end

half_open_cicuit!

Toxiproxy['semian_test_net_http'].downstream(:latency, latency: 2000).apply do
assert_raises Net::ReadTimeout do
http.get('/200')
end
end
end
end

assert_equal expected_read_timeout, http.read_timeout
assert_equal expected_open_timeout, http.open_timeout
end

def test_trigger_open
with_semian_configuration do
with_server do
Expand Down Expand Up @@ -396,6 +432,12 @@ def test_persistent_state_after_server_restart

private

def half_open_cicuit!(backwards_time_travel = 10)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sirupsen I wanted to use the CircuitBreakerHelper but the half_open_circuit! method uses a open_circuit! method with an argument and when I call half_open_circuit! from the net_http_test file, it calls the open_circuit! method inside net_http_test instead of the open_circuit! method inside the helper which raises. To make it work, I would have to change the open_circuit! method name everywhere. What do you think of simply keeping the half_open_cicuit! method in the net_http_test file?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine with me.

Timecop.travel(Time.now - backwards_time_travel) do
open_circuit!
end
end

def with_semian_configuration(options = DEFAULT_SEMIAN_CONFIGURATION)
orig_semian_options = Semian::NetHTTP.semian_configuration
Semian::NetHTTP.instance_variable_set(:@semian_configuration, nil)
Expand Down