Design watchdog, reconnecting to server

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) C

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 centr

StackExchange Code Review Q#112452, answer score: 4