5
\$\begingroup\$

I have console application that sends and process XML data. I have created a fault mechanism that the connection drops. I go in a "fault" mode. And try to reconnect to my server. I'm sending keepalives and so is the server. When I'm not receiving any keepalives and any others messages in a timespan of 2 minutes the programs enters a fault mode. Is this the correct method of doing such operations?

This is my watchdog class (reduced code).

internal class Watchdog
{
    private Timer _timer = new Timer(1000);
    public bool Status
    {
        get { return _status; }
        set
        {
            _status = value;
            OnStatusChanged(new CustomEventArgs(value));
        }
    }

    protected virtual void OnStatusChanged(CustomEventArgs e)
    {
        var handler = StatusChanged;
        if (handler != null) handler(this, e);
    }

    public void Stop()
    {
        _timer.Enabled = false;
    }

    public void SetStatus()
    {
        _latestAction = DateTime.Now;
    }

    public void SetManually()
    {
        _latestAction = DateTime.Now.AddDays(-1); //fool the timer
        Status = true;
    }

    void _timer_Elapsed(object sender, ElapsedEventArgs e)
    {
        if ((DateTime.Now - _latestAction) > TimeSpan.FromSeconds(22))
        {
            Status = true;
        }
        if ((DateTime.Now - _latestAction) < TimeSpan.FromSeconds(22))
        {
            Status = false;
        }
    }
}

This is other class where I call the handler. And try to reconnect.

    void _watchdog_StatusChanged(object sender, CustomEventArgs e)
    {
        if (e.Status) //no message has been processed in two minutes or nothing can be sent, something can be wrong
        {
            _watchdog.Stop(); //stop timer otherwise this method has not enough time to execute and will be recalled
            Console.WriteLine(DateTime.Now + " Error mode initiated!");
            if (!Connection.Connected)
            {
                ((IDisposable) Connection).Dispose();
                openConnection(); //try to reopen conn
                if (Connection.Connected)
                {
                    initialize(); //init all settings
                    _watchdog.SetStatus(); //set live tick
                }
            }
            _watchdog.Start();
        }
        else //fault has been restored.
        {
            Console.WriteLine(DateTime.Now + " Error has been resolved!");
        }
    }

And the event handlers where I receive messages I set a live tick.

    private void _tcpReader_AlarmResponseComplete(object sender, CustomEventArgs e)
    {
        Console.WriteLine("alarmresponse received");
        _processAlarmResponsesToDatabase.Add(e.AlarmResponse);
        _watchdog.SetStatus();
    }

And where I try to send a message

    private void send(string msg) //TODO: must be set to private is now so for testing purpose public
    {
        if (Connection.Connected)
        {
            if (!_tcpReader.IsStopped)
            {
                _writer.Write(msg);
                _writer.Flush();
            }
            else
                Console.Error.WriteLine(
                    "Unable to send message to the server, a request has been made to terminate the connection.");
            _watchdog.SetStatus(); //give live tick to watchdog
        }
        else
        {
            Console.Error.WriteLine("Unable to send message, not connected to the server.");
            _watchdog.Stop(); //the next statement generates an event and the timer would generate the same events wich causes to many events
            _watchdog.SetManually(); //set manually that connection has been terminated
        }
    }
\$\endgroup\$
  • 1
    \$\begingroup\$ SetStatus for a method that doesn't set Status is confusing. You should find another name! \$\endgroup\$ – IEatBagels Dec 1 '15 at 18:27
  • \$\begingroup\$ Get rid of all of your watchdog code and simply do two things. Create a class that provide proper send logic and another that has proper listen logic. Any time an established connection (either sending or receiving) is terminated by the TCP/IP stack, the application is always notified! That is you key to knowing when you need to restart. It takes your solution to being event based rather than timer based. Keep alive logic has no place in TCP/IP.... believe me....please! \$\endgroup\$ – John Peters Dec 2 '15 at 4:42
  • \$\begingroup\$ P.S. If your connections are dropping randomly, this is a sign of a poor network. Don't try to fix that problem in the application layer because it's a losing proposition. Unreliable networks always produce unreliable results and in the end they always out smart the application layer. If you can't do repeated PINGs with 100% responses in reasonable time limits (under 500ms) then your problem is in the network. \$\endgroup\$ – John Peters Dec 2 '15 at 4:44
  • \$\begingroup\$ @Heslacher Does the latest edit address all what was wrong with it or just parts of it? \$\endgroup\$ – Mast Dec 2 '15 at 8:45
  • 3
    \$\begingroup\$ "This is my watchdog class (reduced code)." Don't reduce code for Code Review. That's Stack Overflow style, we do things different here. \$\endgroup\$ – Mast Dec 2 '15 at 8:46
4
\$\begingroup\$

I'll express my thoughts step by step reading your code (just a note: you should post a complete compilable code, at least of your main class Watchdog, if you take out what you think is not relevant then it is much harder to understand what you're doing).

internal class Watchdog : IDisposable

internal modifier (unless Watchdog is a nested class) is superfluous. Moreover if you're not supposed to derive any class from Watchdog I'd declare it as sealed (note that it's an internal class then you can simply remove it when you will need to derive, there is not any public interface to respect).

sealed class Watchdog : IDisposable

IDisposable interface usually comes together with a common pattern implementation, I see no reason to avoid that (even in simple cases):

~Watchdog() {
    Dispose(false);
}

public void Dispose() {
    Dispose(true);
    GC.SuppressFinalize(this);
}

private void Dispose(bool disposing) {
    try {
        if (disposing)
            _timer.Elapsed -= _timer_Elapsed;
    }
    finally {
        _timer.Dispose();
    }
}

private Timer _timer = new Timer(1000);

_timer is initialized only here (or, better, in constructor) then it should be market as readonly:

private const int TimerTickInMilliseconds = 1000;
private readonly Timer _timer;

public Watchdog() {
    _timer = new Timer(TimerTickInMilliseconds);
    _timer.Elapsed += _timer_Elapsed;
}

public bool Status { ... }

In your setter you do not check if value changed, if you repeatedly assign true to Status then you'll fire StatusChanged each time.

public bool Status {
    get {
        return _status;
    }
    set {
        if (_status != value) {
            _status = value;
            OnStatusChanged(EventArgs.Empty);
        }
    }
}

That said Status name is pretty misleading. What does it mean? Does it indicate whether timer is enabled? It actually means watchdog is running and after a fixed amount of time watched operation didn't complete. Make it clear, change its name to, for example, Hanged.

public bool Hanged { ... }

public void SetStatus() { ... }

Your methods have too vague name, as for Status property it should be clear what they do. I'd suggest to rename them:

public void Stop() { ... }
public void Start() { ... }

SetManually() is pretty weird (IMO) but I didn't read caller code (so far) so I'd temporary...drop it. Externally they may simply set Hanged to true.

void _timer_Elapsed(object sender, ElapsedEventArgs e) { ... }

You're performing this check:

if (a > b)
    doThis();
if (a < b)
    doThat();

It may be simplified to (note that we're also handling a == b case).

if (a > b)
    doThis();
else
    doThat();

You're also setting Status (now Hanged) to false but you don't actually need it, its value must be set in Start() method. I'd also rename this method to something slightly more meaningful and move out 22 seconds magic number:

public void Start() {
    _timer.Enabled = true;
    Hanged = false;
}

private void OnTimerTick(object sender, ElapsedEventArgs e) {
    if (DateTime.Now - _watchedActivityStartTime > Timeout)
        Hanged = true;
}

Few notes:

  • Timeout may be private readonly TimeSpan Timeout = TimeSpan.FromSeconds(22) or a public accessible property.
  • You do not need to set Hanged to false (see previous paragraph).
  • I renamed _latestAction to _watchedActivityStartTime trying to make clear what it contains.
  • You may consider (it depends which kind of watchdog you want to implement) to also stop the timer when you detected that task hanged (just add a call to Stop()).
  • If you need to also detect if operation recover (after signaling it was hanged) then you should add this logic in this method (checking Hanged status and adding another DateTime _lastHangOccurence and eventually adding a Reset() method to update _watchedActivityStartTime).

Note that you do not need to perform this check each second, timer can directly be initialized to = new Timer((int)Timeout.TotalMilliseconds) and stopped after hang has been detected (no need to check if (DateTime.Now - _watchedActivityStartTime > Timeout)):

public Watchdog() {
    _timer = new Timer((int)Timeout.TotalMilliseconds);
    _timer.Elapsed += TimerOnElapsed;
}

private void OnTimerTick(object sender, ElapsedEventArgs e) {
    Hanged = true;
    Stop();
}

One final minor note: you should consider to group your methods (somehow), you're now mixing public/protected/private methods/variables in apparently random order.


I skip review of calling code because there isn't (really!) enough context to suggest anything but basic design notes.


All these said I'd consider to change your overall design. As it is now you have an utility class (Watchdog) that is nothing more than a timer. All the logic is spanned between this and callers. A better (IMO) design should centralize all this logic. Just a proof of concept (I omit not relevant code) of one way to do it:

sealed class Watchdog : IDisposable {
    public Watchdog(Action action) {
    }

    public void Start() { 
    }

    public event EventHandler Hanged;
}

Used like this:

var watchdog = new Watchdog(TryConnect);
watchdog.Hanged += OnConnectionHanged;
watchdog.Start();

I said one way because (according to your usage pattern) you may have completely different interfaces (you may, for example, have a Start() method that accepts two delegates). The main point is: remove all watchdog logic from callers, what they have to provide are just two actions to call:

  • One action to execute, this is the watched task.
  • One action to call when watched task hanged.

Everything else must stay inside your Watchdog class.

\$\endgroup\$
  • \$\begingroup\$ I would have to completly rework my classes to support this design of a watchdog class. I took your others notes and reworked my class. \$\endgroup\$ – WagoL Dec 2 '15 at 13:13
  • 1
    \$\begingroup\$ Yes, it's a second refactoring step. No need to do it immediately but something you should consider if you see yourself writing same interaction code again and again. \$\endgroup\$ – Adriano Repetti Dec 2 '15 at 13:18
-1
\$\begingroup\$

Your logic seems really odd. I would start out with my watchdog class like this:

When you rewrite watch dog to this it becomes so close to Timer that you are probably better off just using a timer and just remove the watchdog class completely.

internal class WatchDog : IDisposable
{
    private readonly Timer _timer;

    public WatchDog( TimeSpan timespan )
    {
      _timer = new Timer( timespan.TotalMilliseconds );
      _timer.Elapsed += TimerOnElapsed;
      _timer.Start();
    }

    public void Reset()
    {
      _timer.Stop();
      _timer.Start();
      IsRunning = true;
    }

    private void TimerOnElapsed( object sender, ElapsedEventArgs elapsedEventArgs )
    {
      TimedOut();
      IsRunning = false;
    }

    public bool IsRunning { get; set; }

    public event Action TimedOut = delegate { };

    public void Dispose()
    {
      _timer.Elapsed -= TimerOnElapsed;
      _timer.Dispose();
    }
}
\$\endgroup\$
  • \$\begingroup\$ My two cents: you may consider to implement IDisposable pattern. Your code actually behaves different than OP code. Weird event declaration. IsRunning meant (for OP) hanged not watching. Monitored is not hanged after one timer tick but after a timeout. It may be your timespan parameter (meaningless name) but in this case it won't detect when operation recovered again. \$\endgroup\$ – Adriano Repetti Dec 2 '15 at 10:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.