blankensteiner commented on code in PR #167:
URL: https://github.com/apache/pulsar-dotpulsar/pull/167#discussion_r1280401766
##########
src/DotPulsar/Internal/PingPongHandler.cs:
##########
@@ -58,16 +55,13 @@ private void Watch(object? state)
{
try
{
- var lastCommand = Interlocked.Read(ref _lastCommand);
- var now = Stopwatch.GetTimestamp();
- var elapsed = TimeSpan.FromSeconds((now - lastCommand) /
Stopwatch.Frequency);
- if (elapsed >= _keepAliveInterval)
+ if (_waitForPongResponse)
{
- Task.Factory.StartNew(() => SendPing());
- _timer.Change(_keepAliveInterval, TimeSpan.Zero);
+ _connection.DisposeAsync();
Review Comment:
It is the responsibility of the ConnectionPool to dispose connections. We
can create a way for the PingPongHandler to tell the connection that it is
inactive so that the ConnectionPool will detect it.
##########
src/DotPulsar/Internal/PingPongHandler.cs:
##########
@@ -24,26 +24,23 @@ namespace DotPulsar.Internal;
public sealed class PingPongHandler : IAsyncDisposable
{
private readonly IConnection _connection;
- private readonly TimeSpan _keepAliveInterval;
private readonly Timer _timer;
private readonly CommandPing _ping;
private readonly CommandPong _pong;
- private long _lastCommand;
+ private volatile bool _waitForPongResponse;
Review Comment:
Changing this from a timestamp to a bool will lengthen the reaction time.
Let's say we have a keepalive interval of 60 seconds and that we receive a
message 1 second after the last check, but then the connection is lost.
In the old implementation, we wake up 59 seconds later and see that we have
a timeout in 1 second, so we reschedule to wake up in 1 second and start acting.
With the new implementation, we wake up and see we got a message since the
last check, so we wait 60 seconds again before we realize we need to act.
##########
src/DotPulsar/Internal/PingPongHandler.cs:
##########
@@ -24,26 +24,23 @@ namespace DotPulsar.Internal;
public sealed class PingPongHandler : IAsyncDisposable
{
private readonly IConnection _connection;
- private readonly TimeSpan _keepAliveInterval;
private readonly Timer _timer;
private readonly CommandPing _ping;
private readonly CommandPong _pong;
- private long _lastCommand;
+ private volatile bool _waitForPongResponse;
public PingPongHandler(IConnection connection, TimeSpan keepAliveInterval)
{
_connection = connection;
- _keepAliveInterval = keepAliveInterval;
_timer = new Timer(Watch);
- _timer.Change(_keepAliveInterval, TimeSpan.Zero);
+ _timer.Change(keepAliveInterval, keepAliveInterval);
Review Comment:
This can cause multiple simultaneous Watch invocations, which is why I set
it explicitly and thereby also set it for the actual time we want/need to wait.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
