BrandonPotter / SimpleTCP

Straightforward .NET library to handle the repetitive tasks of spinning up and working with TCP sockets (client and server).
Apache License 2.0
365 stars 108 forks source link

ClientDisconnected event is not thrown #8

Open Henna1977 opened 8 years ago

Henna1977 commented 8 years ago

The ClientDisconnected Event is not raised at the host.

I have used some differnt TCP Clients to connect to a simple host impementation. I didn't get any Information about the Client disconnect.

Here is the code I have written.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Net;
using System.Net.Sockets;
using System.Text;
using System.Threading;
using System.Threading.Tasks;

using SimpleTCP;

namespace SimpleHost
{
    public class Host: IDisposable  
    {
        private const int SERVER_PORT = 1100;
        private const string EOT_MARK = "\n\r";

        private SimpleTcpServer _tcpHost = new SimpleTcpServer();
        private TcpClient _connectedClient = null;

        public Host()
        {
            _tcpHost.ClientConnected += _tcpHost_OnClientConnected;
            _tcpHost.ClientDisconnected += _tcpHost_OnClientDisconnected;
            _tcpHost.DataReceived += _tcpHost_OnDataReceived;
            _tcpHost.AutoTrimStrings = true;
            StartListening();
        }

        private void _tcpHost_OnDataReceived(object sender, Message e)
        {
            Console.WriteLine("Server received: {0}", e.MessageString);

            e.Reply("Echo\n\r");

        }

        private void _tcpHost_OnClientDisconnected(object sender, TcpClient e)
        {
            Console.WriteLine("Client {0} disconnected", e.Client.RemoteEndPoint.ToString());
            if (_connectedClient == e)
            {
                _connectedClient.Dispose();
                _connectedClient = null;
            }

        }

        private void _tcpHost_OnClientConnected(object sender, TcpClient e)
        {
            Console.WriteLine("Client {0} connected", e.Client.RemoteEndPoint.ToString());
            if (_connectedClient == null)
            {
                _connectedClient = e;
            }
            else
            {
                e.Dispose();
            }

        }

        private void StartListening()
        {

            _tcpHost.Start(SERVER_PORT);
        }

        public void Dispose()
        {
            if (_connectedClient !=null)
            {
                _connectedClient.Dispose();
            }
            _tcpHost.Stop();
            _tcpHost = null;
        }
    }
}
bigworld12 commented 7 years ago

i am having the same issue,i tried even calling Disconnect manually and nothing

pierre1977 commented 7 years ago

You can fix this by remove the lines 102-108 in "ServerListener.cs". The "RunLoopStep()" never comes to disconnectedClients.

BrandonPotter commented 7 years ago

This is unfortunately a tough problem to solve for TCP all around.

If you're not sending traffic, you can't clearly determine if a client has disconnected.

Best solution I've found so far is to add an "ignore" character on the client side that does not get processed, and periodically send the ignored character to the client. Basically, a heartbeat byte. If this were enabled by default, this could create some interesting issues for non-SimpleTCP clients, so it would have to be an opt-in property. Thoughts?

bigworld12 commented 7 years ago

I have used the pattern (id, packet length,packet content) in the past, it's actually safer to handle because I always know what I will read. to solve the disconnect notification issue, I would create a ping packet with an id 0, packet length 0 and empty content. then even for non-SimpleTCP clients, if they follow that same pattern, they can ignore any packet with the id 0.

BrandonPotter commented 7 years ago

Well, all it would take is a single byte; don't want to change the functionality of SimpleTCP to be a ID/length/payload/checksum type protocol, because the library is meant to be used for interfacing with simple systems that don't support that; I think Protobuf covers that type of payload pretty well.

In any case, as far as heartbeat goes... This would need to be an opt-in feature that you configure on both the server and the client so that the protocol can be handled correctly. Don't want someone to update the library on the server side and suddenly their telnet clients start getting weird smiley faces every 10 secs...

The single heartbeat byte would need to default to the least likely byte that someone would legitimately send; I don't know if there's a specific ASCII or UTF-8 character meant for heartbeats though, suggestions?

brudo commented 7 years ago

One option for the heartbeat message is it could be the message terminator byte alone. Then it is up to the client to ignore blank messages. But, just in case clients cannot handle that, it could be off by default.

I suggest having the time interval between server heartbeats controllable by a property as well. If the heartbeat interval was 0 (the default) then no heartbeats.

bigworld12 commented 7 years ago

maybe you can use some of the limit bytes like 255 or something like that

elitefuture commented 6 years ago

don't send data periodically, instead just use the built in timeout feature on a client to check, set the timeout once the client connects.