search

davidfowl / BedrockFramework

High performance, low level networking APIs for building custom servers and clients.
MIT License
1.07k stars 155 forks source link

Support adding/removing socket listeners on already running servers #159

Open jooooel opened 1 year ago

jooooel commented 1 year ago

I'm working on adding support for starting and stopping individual services in Project Tye, without restarting Tye entirely. One thing I'm missing is support for adding and removing socket listeners to an already running BedrockFramework server (needed by the ProxyService in Tye).

I've taken a shot at implementing that here, with the hope that it could get released into a new nuget version and be used in my fork of Tye.

I took the liberty to add some tests for this new behaviour. Maybe it's arguable if they are unit or integration tests (because of the use of sockets etc), but they seem to run fine.

Hope you're open to this change, and please let me know if you'd like me to change it it any way 😊

davidfowl commented 1 year ago

This needs to be thread safe.

jooooel commented 1 year ago

@davidfowl Oh, you're right! Didn't think about that :) I don't often come across areas where I have to think about thread safety in my daily work, but I've given it a go here. Let me know if there are any gotchas or something special to look out for...

And I'm not really sure how to solve this the best way. I started out with a ConcurrentDictionary for the RunningListeners, but I soon realised that wasn’t enough. So I went for a simple locking instead. This way, the server will have to wait until any Add/Remove operation has finished, and vice versa. I took a look at how Kestrel handles similar cases, and was inspired by that.

I think I need a bit of help reasoning about the heartbeat timer, if there are possible concurrency issues there as well. I guess the risk is that a listener currently/already being stopped receives a call to TickHeartBeat(). However, this isn’t being handled while the server is shutting down in the current implementation anyway, so maybe it’s not something to bother about?

jooooel commented 1 year ago

But basically all I did was to await a SemaphoreSlim in places where the RunningListeners are handled. And store them in a Dictionary to simplify lookups.

jooooel commented 1 year ago

And by the way. Do with this what you will 😊 I’m not expecting you to spend any of your free time on me and my tinkering