Remove dead clients from server (v2)

[martbock]

This pull request replaces #34.

I moved the client handling logic over to the clients.go file. In there, I created a StartClient function that now takes care of starting the incoming and outgoing handlers.

I also introduced a WaitGroup per client. Thus, the StartClient function can wait until both handlers are done to remove the client from the slice containing all active clients. To avoid blocking the message broadcasting loop, I added a default case to the channel sending after I learned that there is such a thing as non-blocking channel sending 1. After I had introduced the default case, I noticed that lots of messages were discarded because the channel was unbuffered.

So I added a buffer of 100 messages to each client's outgoing channel. I don't know if that's enough though, I didn't test it under load.

How does the WaitGroup work?
When the websocket connection breaks because the client disappears (shouldn't rely on graceful closing), the blocking readMessages call in the incoming handler returns an error. The handler exits and calls waitGroup.Done(). When the outgoing handler tries to send the next message, the WriteMessage call will also return an error, making the outgoing handler to exit and call waitGroup.Done(). When both handers finished, the blocking waitGroup.Wait() call in the StartClient function releases and the client is removed from the active clients slice.

TL;DR:
Dead clients are successfully removed now.

Fix #11

[MerzMax]

I am not quiet sure if i like the refactoring... In my opinion the client now contains logic that is on the one side the connection stuff to one client (as i would expect) but on the other side there is logic in there for maintaining all clients ... Would it make sense to introduce a new layer of abstraction in between the server and the client that handles all connections clients? Something like a message broker.

[martbock]

I am not quiet sure if i like the refactoring... In my opinion the client now contains logic that is on the one side the connection stuff to one client (as i would expect) but on the other side there is logic in there for maintaining all clients ...

Do you think it's better the way it was before? In my opinion, it's an improvement to consolidate the client and broadcasting logic into the one file instead of having randomly sprinkled pieces of that code within the main.go file.

Would it make sense to introduce a new layer of abstraction in between the server and the client that handles all connections clients? Something like a message broker.

Maybe we don't even need an abstraction layer, how about we put the client management part into a separate file? Can we open a new issue for a refactoring? I think this is outside of this issue's scope. At least in my opinion, it's better now than it was before.

[MerzMax]

I am not quiet sure if i like the refactoring... In my opinion the client now contains logic that is on the one side the connection stuff to one client (as i would expect) but on the other side there is logic in there for maintaining all clients ...

Do you think it's better the way it was before? In my opinion, it's an improvement to consolidate the client and broadcasting logic into the one file instead of having randomly sprinkled pieces of that code within the main.go file.

The previous way separated the serverlogic (client managenent and broadcasting) from the client parts that send and receive the messages. Your refactoring mixes this two parts.

Would it make sense to introduce a new layer of abstraction in between the server and the client that handles all connections clients? Something like a message broker.

Maybe we don't even need an abstraction layer, how about we put the client management part into a separate file? Can we open a new issue for a refactoring? I think this is outside of this issue's scope. At least in my opinion, it's better now than it was before.

Yes, I think that is a good idea! :) I will open a new issue for this.