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

Failed heartbeats will longer raise exceptions #565

Merged
merged 1 commit into from
Aug 23, 2018

Conversation

sbonebrake
Copy link

Addresses #559
A test can be added if this is agreed to be the correct approach.

@michaelklishin
Copy link
Member

@sbonebrake I'm a bit concerned that we might leave HeartbeatSender threads around between recoveries. Will need to do some testing/profiling. Otherwise I think this solution is OK. Thank you!

@sbonebrake
Copy link
Author

I think it's fine to have the HeartbeatSender thread still running during recovery as long as it fails silently. I think it's probably a good idea to add an open? check to transport#write_without_timeout.

def write_without_timeout(data, raise_exceptions = false)
  begin
    if open?
      @writes_mutex.synchronize { @socket.write(data) }
      @socket.flush
     end
    rescue SystemCallError, Bunny::ConnectionError, IOError => e
      close
    end
  end
end

Thoughts?

@michaelklishin
Copy link
Member

I'd prefer stopping heartbeat senders since recovery won't be happening more than once every few seconds (worst case), so instantiating a new one and starting a new thread would not be a problem.

@sbonebrake
Copy link
Author

I just realized that if the heartbeat threads stopped on failure then they would rely on automatically_recover, but that option may be false. Eg { threaded: false, automatically_recover: false }
Updated my PR with a toxiproxy test case

@michaelklishin michaelklishin merged commit 7769938 into ruby-amqp:master Aug 23, 2018
@michaelklishin
Copy link
Member

Thank you!

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