Skip to content

Commit

Permalink
Stop reconnect_on_network_failure loop on session#close
Browse files Browse the repository at this point in the history
reopen session in case automatically_recover is called after close

make explicit close handling more obvious on recovery
  • Loading branch information
andreaseger committed Mar 6, 2020
1 parent ec70498 commit 13b98e9
Show file tree
Hide file tree
Showing 4 changed files with 28 additions and 2 deletions.
6 changes: 6 additions & 0 deletions lib/march_hare/exceptions.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,12 @@ class NetworkException < Exception
class ConnectionRefused < NetworkException
end

class ConnectionClosedException < Exception
def initialize(message='')
super("Connection was explicitly closed and cannot be reopened. Create a new Connection instead. #{message}")
end
end

class ChannelLevelException < Exception
attr_reader :channel_close

Expand Down
8 changes: 7 additions & 1 deletion lib/march_hare/session.rb
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,7 @@ def initialize(connection_factory, opts = {})
@shutdown_hooks = Array.new
@blocked_connection_hooks = Array.new
@connection_recovery_hooks = Array.new
@was_explicitly_closed = false

if @automatically_recover
self.add_automatic_recovery_hook
Expand Down Expand Up @@ -225,8 +226,11 @@ def close
ch.close
end

@was_explicitly_closed = true
maybe_shut_down_executor
@connection.close
rescue com.rabbitmq.client.AlreadyClosedException
@logger.debug("close: connection already closed")
end

# @return [Boolean] true if connection is open, false otherwise
Expand Down Expand Up @@ -318,7 +322,8 @@ def disable_automatic_recovery
# Begins automatic connection recovery (typically only used internally
# to recover from network failures)
def automatically_recover
@logger.debug("session: begin automatic connection recovery")
raise ConnectionClosedException if @was_explicitly_closed
@logger.debug("session: begin automatic connection recovery #{Thread.current.inspect}")

fire_recovery_start_hooks

Expand Down Expand Up @@ -583,6 +588,7 @@ def converting_rjc_exceptions_to_ruby(&block)
def reconnecting_on_network_failures(interval_in_ms, &fn)
@logger.debug("session: reconnecting_on_network_failures")
begin
return if @was_explicitly_closed
fn.call
rescue IOError, MarchHare::ConnectionRefused, java.io.IOException, java.util.concurrent.TimeoutException
java.lang.Thread.sleep(interval_in_ms)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

def close_all_connections!
# wait for stats to refresh, make sure to run bin/ci/before_build.sh as well!
sleep 0.7
sleep 1.7
http_client.list_connections.each do |conn_info|
puts "Closing connection #{conn_info.name}..."
http_client.close_connection(conn_info.name)
Expand Down
14 changes: 14 additions & 0 deletions spec/higher_level_api/integration/connection_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,16 @@
end
end

context "when connection manually closed" do
it "raises an exception on #automatically_recover" do
c = MarchHare.connect(uri: "amqp://127.0.0.1")
c.close
expect {
c.automatically_recover
}.to raise_error(MarchHare::ConnectionClosedException)
end
end

it "handles amqp:// URIs w/o path part" do
c = MarchHare.connect(uri: "amqp://127.0.0.1")

Expand All @@ -61,6 +71,7 @@
end
c = MarchHare.connect(executor_factory: factory, network_recovery_interval: 0)
c.close
c.instance_variable_set(:@was_explicitly_closed, false)

This comment has been minimized.

Copy link
@michaelklishin

michaelklishin Mar 6, 2020

Member

Please introduce a writer for this ivar.

This comment has been minimized.

Copy link
@andreaseger

andreaseger Mar 6, 2020

Author Contributor

ok, can change this later.
How about maybe adding something like reopen (which would reset the ivar + call automatically_recover) instead?

This comment has been minimized.

Copy link
@michaelklishin

michaelklishin Mar 6, 2020

Member

Good idea!

c.automatically_recover
c.close
expect(calls).to eq(2)
Expand All @@ -72,6 +83,7 @@
expect(c).to be_connected
c.close
expect(c).not_to be_connected
c.instance_variable_set(:@was_explicitly_closed, false)
c.automatically_recover
sleep 0.5
expect(c).to be_connected
Expand All @@ -93,6 +105,7 @@
expect(c).to be_connected
c.close
expect(c).not_to be_connected
c.instance_variable_set(:@was_explicitly_closed, false)
c.automatically_recover
sleep 0.5
expect(c).to be_connected
Expand All @@ -104,6 +117,7 @@
expect(c).to be_connected
c.close
expect(c).not_to be_connected
c.instance_variable_set(:@was_explicitly_closed, false)
c.automatically_recover
sleep 0.5
expect(c).to be_connected
Expand Down

0 comments on commit 13b98e9

Please sign in to comment.