Improved Heartbeat Write Timer Handling

[ashneilson]

• Resolves an issue with the Heartbeat Write Timer repeatedly firing and blocking on the sychronization lock. The timer will now fire once and be restarted (if not disposed) at the end of method processing.

Proposed Changes

Modified synchronization and parameters of the Write Heartbeat Timer to ensure it can only fire once. It will then be started again at the end of the callback.

This resolves an issue that can occur when writing a heartbeat frame to the socket fails (e.g. broken connection) and the Write Heartbeat Timer continued to fire and block on the lockable _heartbeatWriteLock object. Over time this would eventually exhaust the ThreadPool if the socket write didn't return.

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation (correction or otherwise)
• Cosmetics (whitespace, appearance)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask on the
mailing list. We're here to help! This is simply a reminder of what we are
going to look for before merging your code.

• I have read the CONTRIBUTING.md document
• I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
• All tests pass locally with my changes
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)
• Any dependent changes have been merged and published in related repositories

Further Comments

[ashneilson]

It appears builds are failing due to configuration issues?

[michaelklishin]

@ash-ricado you are correct, CI needs propping every so often. We will QA this, don't worry.

[michaelklishin]

@ash-ricado any way we can reasonably reliably reproduce this?

[kjnilsson]

I think this looks fine to me apart from the interval change which I don't know the reason for.

[kjnilsson]

Why the change from 200 to 300 here?

[ashneilson]

I simply changed this to prevent both the Read and Write Timers firing at the same time. I was considering using Random.Next(100,300) to ensure that with multiple connections, the timers will fire at different times.

Not required to fix the issue this PR deals with. More than happy to remove if desired.

[michaelklishin]

We strongly discourage heartbeat timeouts < 5 seconds so any timer interval < 1s should be reasonable, and anything < 500 ms is optimal IMO.

[michaelklishin]

This looks reasonable. @ash-ricado please undo the interval change (or justify it) and provide some detail on the problem this PR addresses.

[michaelklishin]

Let's wait for some extra details first.

[ashneilson]

@michaelklishin Apologies for the delay in my response.

Problem solved by this PR
Resolves an issue that can occur when writing a heartbeat frame to the socket fails (e.g. broken connection) and the Write Heartbeat Timer continues to fire and block on the lockable _heartbeatWriteLock object. Over time this would eventually exhaust the ThreadPool if the socket write didn't return.

Identified by our use of remote nodes that talk to a RabbitMQ Cluster over connections that can sometimes become unstable. Monitoring allowed us to see the ThreadPool increase over a period of an hour until the maximum number of threads was consumed.

Should be able to reliably reproduce by publishing data (12K or larger) to a channel every 5 seconds, then drop the network interface that provides access to the RabbitMQ Server. The Heartbeat Write Timer should continue to fire and wait on the lock.

NOTE: This does rely on the socket hanging after an unclean network disconnection. If the socket is able to be cleanly shutdown, the attempt to write frames will return immediately.

[michaelklishin]

OK, so injecting a latency spike with Toxiproxy or similar would do. Thank you.