[Pull create request] Listener logic

[notgiven688]

Hello!

At the moment we have some rather unnecessary "RestartAfterListenError" property in the code. In my opionion this is horrible. This property was patched in because the server socket stopped listening if an error occurred during a client connection being established.

Let us see why this happens:

(1) In WebsocketServer.cs in the Start() function we call ListenForClients(); This initiates the whole "accept new clients" logic.
(2) In ListenForClients() we call ListenerSocket.Accept(..) which itself calls the Accept method in the SocketWrapper class. Let's take a look into the corresponding code:

        public Task<ISocket> Accept(Action<ISocket> callback, Action<Exception> error)
        {
            Func<IAsyncResult, ISocket> end = r => _tokenSource.Token.IsCancellationRequested ? null : new SocketWrapper(_socket.EndAccept(r));
            var task = _taskFactory.FromAsync(_socket.BeginAccept, end, null);
            task.ContinueWith(t => callback(t.Result), TaskContinuationOptions.OnlyOnRanToCompletion)
                .ContinueWith(t => error(t.Exception), TaskContinuationOptions.OnlyOnFaulted);
            task.ContinueWith(t => error(t.Exception), TaskContinuationOptions.OnlyOnFaulted);
            return task;
        }

Whenever a new client connects, the client is accepted by the socket. If this succeeds we call the callback passed to this method. If it fails we call the error callback. We also call the error callback if the callback throws an error itself. Quite complicated, isn't it?

(3) okay, if everything went fine, "callback" is called after a new socket was accepted. We should now proceed to accept new/more clients. How do we do that? We call ListenForClients() within the callback method - which brings us to the beginning of this circle (2).

The flaws of this (1),(2),(3) step method is that it is rather complicated and error prone. The whole logic fails if for example an error is thrown in the callback method itself (note that the current code does not differentiate between errors from accepting new clients and errors thrown in the callback called afterwards). An example for such an error is in the line

FleckLog.Debug(String.Format("Client connected from {0}:{1}", clientSocket.RemoteIpAddress, clientSocket.RemotePort.ToString()));

in OnClientConnect() (which is the actual callback-function used). Under some circumstances clientSocket.RemoteIpAddress can throw an exception - and kills the complete server-logic. We throw an error and restart the listener socket although there was nothing wrong with the listener socket - it is just a client which maybe closed a connection really early.

Since I am too stupid to create real pull-requests someone has to do it for me. Here are my proposed changes:
https://github.com/notgiven688/Fleck/commit/79a4f995fb5aaa6204605ab641a069a8cf6a2764

and this

https://github.com/notgiven688/Fleck/commit/b3bfaa69e6edfe7788ac182b79e8d2d87f6291a2

[statianzo]

This is a great call out. I agree this is a cleaner approach than throwing away the listener for a client error. I don't have a .net environment to build this out, but would definitely accept a pull request from any other contributors. Thanks for your contribution, @notgiven688.

[darkl]

Created pull request #226.