Skip to content

Improved Heartbeat Write Timer Handling #636

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.

Sign up for GitHub

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

Merged
michaelklishin merged 1 commit into from
Apr 8, 2019
Merged

Improved Heartbeat Write Timer Handling #636

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
93 changes: 61 additions & 32 deletions projects/client/RabbitMQ.Client/src/client/impl/Connection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1015,15 +1015,32 @@ public void MaybeStartHeartbeatTimers()
m_hasDisposedHeartBeatReadTimer = false;
m_hasDisposedHeartBeatWriteTimer = false;
#if NETFX_CORE
_heartbeatWriteTimer = new Timer(HeartbeatWriteTimerCallback);
_heartbeatReadTimer = new Timer(HeartbeatReadTimerCallback);
_heartbeatWriteTimer.Change(200, (int)m_heartbeatTimeSpan.TotalMilliseconds);
_heartbeatReadTimer.Change(200, (int)m_heartbeatTimeSpan.TotalMilliseconds);
lock(_heartBeatWriteLock)
{
_heartbeatWriteTimer = new Timer(HeartbeatWriteTimerCallback);
_heartbeatWriteTimer.Change(200, Timeout.Infinite);
}

lock (_heartBeatReadLock)
{
_heartbeatReadTimer = new Timer(HeartbeatReadTimerCallback);

_heartbeatReadTimer.Change(300, Timeout.Infinite);
Copy link
Contributor

@kjnilsson kjnilsson Apr 5, 2019

Choose a reason for hiding this comment

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

Why the change from 200 to 300 here?

Copy link
Contributor Author

@ashneilson ashneilson Apr 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@michaelklishin michaelklishin Apr 8, 2019

Choose a reason for hiding this comment

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

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

}
#else
_heartbeatWriteTimer = new Timer(HeartbeatWriteTimerCallback, null, 200, (int)m_heartbeatTimeSpan.TotalMilliseconds);
_heartbeatReadTimer = new Timer(HeartbeatReadTimerCallback, null, 200, (int)m_heartbeatTimeSpan.TotalMilliseconds);
_heartbeatWriteTimer.Change(TimeSpan.FromMilliseconds(200), m_heartbeatTimeSpan);
_heartbeatReadTimer.Change(TimeSpan.FromMilliseconds(200), m_heartbeatTimeSpan);
lock (_heartBeatWriteLock)
{
_heartbeatWriteTimer = new Timer(HeartbeatWriteTimerCallback, null, Timeout.Infinite, Timeout.Infinite);

_heartbeatWriteTimer.Change(200, Timeout.Infinite);
}

lock (_heartBeatReadLock)
{
_heartbeatReadTimer = new Timer(HeartbeatReadTimerCallback, null, Timeout.Infinite, Timeout.Infinite);

_heartbeatReadTimer.Change(300, Timeout.Infinite);
}
#endif
}
}
Expand All @@ -1050,7 +1067,9 @@ public void HeartbeatReadTimerCallback(object state)
{
return;
}

bool shouldTerminate = false;

try
{
if (!m_closed)
Expand Down Expand Up @@ -1111,37 +1130,47 @@ public void HeartbeatWriteTimerCallback(object state)
{
return;
}
bool shouldTerminate = false;
}

bool shouldTerminate = false;

try
{
try
{
try
{
if (!m_closed)
{
WriteFrame(m_heartbeatFrame);
}
}
catch (Exception e)
if (!m_closed)
{
HandleMainLoopException(new ShutdownEventArgs(
ShutdownInitiator.Library,
0,
"End of stream",
e));
shouldTerminate = true;
WriteFrame(m_heartbeatFrame);
}
}
catch (Exception e)
{
HandleMainLoopException(new ShutdownEventArgs(
ShutdownInitiator.Library,
0,
"End of stream",
e));
shouldTerminate = true;
}

if (m_closed || shouldTerminate)
{
TerminateMainloop();
FinishClose();
}
if (m_closed || shouldTerminate)
{
TerminateMainloop();
FinishClose();
}
catch (ObjectDisposedException)
}
catch (ObjectDisposedException)
{
// timer is already disposed,
// e.g. due to shutdown
} /**/

lock(_heartBeatWriteLock)
{
if(m_closed == false && shouldTerminate == false && _heartbeatWriteTimer != null)
{
// timer is already disposed,
// e.g. due to shutdown
} /**/
_heartbeatWriteTimer.Change((int)m_heartbeatTimeSpan.TotalMilliseconds, Timeout.Infinite);
}
}
}

Expand Down