ハートビート(ハックソック)


5

私は最近、サーバープロジェクトとクライアントアプリケーションの間のシンプルなハートビートを提供するために小さなプロジェクトに取り組んでいます。かなり満足し、機能が向上するかどうか疑問に思っています(クリーニングを使用できることを知っているので、標準の命名規則を使用していますが、すばやく簡単にコーディングしてクリーンアップします)

助言をいただければ幸いです。

Haksock.cs

using System;
using System.Net;
using System.Net.Sockets;
using System.Text;

namespace HakSock {
  public class Heart : IDisposable{
    public sealed class ConnectionStateEventArgs : EventArgs {
        public ConnectionState ConnectionState { get; private set; }
        public ConnectionStateEventArgs(ConnectionState connectionState) {
            ConnectionState = connectionState;
        }
    }
    public event EventHandler<ConnectionStateEventArgs> ConnectionStateChanged;
    #region Globals
    private System.Timers.Timer Timer = new System.Timers.Timer();
    private IPEndPoint ipep;
    bool isServer;
    public bool connected = false;
    private byte[] buffer = new byte[1024];
    private DateTime lastUpdate;
    private Socket udpServerSocket;
    private EndPoint ep;
    public TimeSpan timeSinceLastHeartbeat;
    private ConnectionState _connectionState = ConnectionState.Disconnected;
    public ConnectionState ConnectionState {
        get { return _connectionState; }
        set {
            if (value != _connectionState) {
                _connectionState = value;
                var tmp = ConnectionStateChanged;
                if (tmp != null)
                    tmp(this, new ConnectionStateEventArgs(value));
            }
        }
    }
    public bool IsDisposed {
        get;
        private set;
    }
    #endregion

    #region constructor/destructor
    /// <summary>
    /// Initialises a new instance of a Heart
    /// </summary>
    /// <param name="isserver">Specifies whether this is the server</param>
    /// <param name="autostart">specifies if the Heart should start on creation</param>
    /// <param name="port">Specifies the port (this will set the server port if isserver is true, else will set the client port)</param>
    public Heart(bool isserver, bool autostart, int port = 10000) {
        ;
        Timer.Interval = 1500;
        Timer.AutoReset = true;
        isServer = isserver;
        this.ConnectionStateChanged += Heart_ConnectionStateChanged;
        if (isserver) {
            ep = new IPEndPoint(IPAddress.Any, port);
            Timer.Elapsed += srTimer_Elapsed;
            udpServerSocket = new Socket(AddressFamily.InterNetwork, SocketType.Dgram, ProtocolType.Udp);
            udpServerSocket.Bind(ep);
            udpServerSocket.BeginReceiveFrom(buffer, 0, 1024, SocketFlags.None, ref ep, new AsyncCallback(ReceiveData), udpServerSocket);
            udpServerSocket.Ttl = 255;
        } else {
            Timer.Elapsed += clTimer_Elapsed;
            ipep = new IPEndPoint(IPAddress.Loopback, port);
        }
        if (autostart)
            startBeating();

    }

    private void Heart_ConnectionStateChanged(object sender, ConnectionStateEventArgs e) {
        if (e.ConnectionState == ConnectionState.Connected)
            connected = true;
        else
            connected = false;
    }

    /// <summary>
    /// Initialises a new instance of a Heart
    /// </summary>
    /// <param name="timespan">Specifies the timespan between each beat</param>
    /// <param name="isserver">Specifies whether this is the server</param>
    /// <param name="autostart">specifies if the Heart should start on creation</param>
    /// <param name="port">Specifies the port (this will set the server port if isserver is true, else will set the client port)</param>
    public Heart(double timespan = 1500, bool isserver = false, bool autostart = true,int port = 10000) {
        Timer.Interval = timespan;
        isServer = isserver;
        ep = new IPEndPoint(IPAddress.Any, port);
        if (isserver) {
            Timer.Elapsed += srTimer_Elapsed;
            udpServerSocket = new Socket(AddressFamily.InterNetwork,SocketType.Dgram, ProtocolType.Udp);
            udpServerSocket.Bind(ep);
            udpServerSocket.BeginReceiveFrom(buffer, 0, 1024, SocketFlags.None, ref ep, new AsyncCallback(ReceiveData), udpServerSocket);
            udpServerSocket.Ttl = 255;
        } else {
            Timer.Elapsed += clTimer_Elapsed;
            ipep = new IPEndPoint(IPAddress.Loopback, port);
        }
        if (autostart)
            startBeating();
    }
    #endregion

    #region Events

    #region handled events
    private void clTimer_Elapsed(object sender, System.Timers.ElapsedEventArgs e) {
        try {
            SendUdpPacket();
            ConnectionState = ConnectionState.Connected;
        } catch {
            ConnectionState = ConnectionState.Disconnected;
        }
    }
    private void srTimer_Elapsed(object sender, System.Timers.ElapsedEventArgs e) {
        // Calculate the Timespan since the Last Update from the Client.
        timeSinceLastHeartbeat = DateTime.Now.ToUniversalTime() - lastUpdate;

        // Set Lable Text depending of the Timespan
        if (timeSinceLastHeartbeat > TimeSpan.FromMilliseconds(Timer.Interval))
            ConnectionState = ConnectionState.Disconnected;
        else
            ConnectionState = ConnectionState.Connected;
    }

    protected void Dispose(bool disposing) {
        if (disposing) {
            IsDisposed = true;
        }
        Dispose(disposing);
    }
    public void Dispose() {
        this.stopBeating();
    }
    #endregion
    #endregion

    #region Beat
    /// <summary>
    /// Start the Hearbeat
    /// </summary>
    public void startBeating() {
        Timer.Start();
    }
    /// <summary>
    /// Stop the Heartbeat
    /// </summary>
    public void stopBeating() {
        Timer.Stop();
    }
    /// <summary>
    /// Sends a packet to the HeartServer
    /// </summary>
    private void SendUdpPacket() {
        byte[] data = new byte[1024];
        Socket udpClientSocket = new Socket(AddressFamily.InterNetwork, SocketType.Dgram, ProtocolType.Udp);
        data = Encoding.ASCII.GetBytes("lubdub");
        udpClientSocket.SendTo(data, 0, data.Length, SocketFlags.None, ipep);
    }
    /// <summary>
    /// Recieves a packet from the HeartClient
    /// </summary>
    /// <param name="iar"></param>
    void ReceiveData(IAsyncResult iar) {
        byte[] buffer = new byte[1024];
        IPEndPoint sender = new IPEndPoint(IPAddress.Any, 0);
        EndPoint tempRemoteEP = (EndPoint)sender;
        Socket remote = (Socket)iar.AsyncState;
        int recv = remote.EndReceiveFrom(iar, ref tempRemoteEP);
        string stringData = Encoding.ASCII.GetString(buffer, 0, recv);
        Console.WriteLine(stringData);
        lastUpdate = DateTime.Now.ToUniversalTime();
        if (!this.IsDisposed) {
            udpServerSocket.BeginReceiveFrom(buffer, 0, 1024, SocketFlags.None, ref ep, new AsyncCallback(ReceiveData), udpServerSocket);
        }
    }

    private class ConnectionStateChangedEventArgs {
        private ConnectionState value;

        public ConnectionStateChangedEventArgs(ConnectionState value) {
            this.value = value;
        }
    }
    #endregion

}
}

Enums.cs

namespace HakSock {
 public enum ConnectionState {
    Connected,
    Disconnected
 }
}
3

Your code would have a bug related to the protected Dispose(bool) method

protected void Dispose(bool disposing) {
    if (disposing) {
        IsDisposed = true;
    }
    Dispose(disposing);
}  

if you would use it. Luckily you don't use it, otherwise you would get likely get some kind of overflow exception.

So please remove this method so you won't trap inside the trap.

At a second thought it may be better to keep it after doing it right, because the ReceiveData() method relies on the IsDisposed property.


private void Heart_ConnectionStateChanged(object sender, ConnectionStateEventArgs e) {
    if (e.ConnectionState == ConnectionState.Connected)
        connected = true;
    else
        connected = false;
}  

this should be simplified to just

private void Heart_ConnectionStateChanged(object sender, ConnectionStateEventArgs e) {
    connected = e.ConnectionState == ConnectionState.Connected;
}  

The Heart class could use some constructor chaining, which removes some duplicated code.

So this

public Heart(bool isserver, bool autostart, int port = 10000) {
    ;
    Timer.Interval = 1500;
    Timer.AutoReset = true;
    isServer = isserver;
    this.ConnectionStateChanged += Heart_ConnectionStateChanged;
    if (isserver) {
        ep = new IPEndPoint(IPAddress.Any, port);
        Timer.Elapsed += srTimer_Elapsed;
        udpServerSocket = new Socket(AddressFamily.InterNetwork, SocketType.Dgram, ProtocolType.Udp);
        udpServerSocket.Bind(ep);
        udpServerSocket.BeginReceiveFrom(buffer, 0, 1024, SocketFlags.None, ref ep, new AsyncCallback(ReceiveData), udpServerSocket);
        udpServerSocket.Ttl = 255;
    } else {
        Timer.Elapsed += clTimer_Elapsed;
        ipep = new IPEndPoint(IPAddress.Loopback, port);
    }
    if (autostart)
        startBeating();

}

would become this

public Heart(bool isserver, bool autostart, int port = 10000) 
        :this(1500, isserver, autostart, port)
{}

Seeing something like this

private System.Timers.Timer Timer = new System.Timers.Timer();
private IPEndPoint ipep;
bool isServer;  

just doesn't look right. You should always add access modifiers to your properties and methods making the indent more clear.


Please read regarding the use of regions: Are #regions an antipattern or code smell?


public ConnectionState ConnectionState {
    get { return _connectionState; }
    set {
        if (value != _connectionState) {
            _connectionState = value;
            var tmp = ConnectionStateChanged;
            if (tmp != null)
                tmp(this, new ConnectionStateEventArgs(value));
        }
    }
}  

I don't really like that setter, it is doing IMO to much. You should consider to extract the raising of the event to a separate method like so

protected void OnConnectionStateChanged(ConnectionStateEventArgs e)
{
    var stateChanged = ConnectionStateChanged;
    if (stateChanged != null)
    {
        stateChanged(this, e));
    }
}  

to be called form the setter like so

    set {
        if (value != _connectionState) {
            OnConnectionStateChanged(new ConnectionStateEventArgs(value));
        }
    }  

private class ConnectionStateChangedEventArgs {
    private ConnectionState value;

    public ConnectionStateChangedEventArgs(ConnectionState value) {
        this.value = value;
    }
}  

You should make the ConnectionState value readonly.


Using braces {} although they might be optional will make your code less error prone.