search

DarkRiftNetworking / Hazel-Networking

Hazel Networking is a low level networking library for C# providing connection orientated, message based communication via TCP, UDP and RUDP.
http://www.darkriftnetworking.com
MIT License
206 stars 44 forks source link

Implement Keep Alive packets for TCP #10

Open brogowski opened 6 years ago

brogowski commented 6 years ago

Closes #7

I've improved a bit tests for keep alive packets. Server tests will now correctly fail - previously was hanging forever.

I've changed keep alive packets to be send only after successful connection. Previously, after 1000ms from creating new Connection object, keep alive will cause exception.

Lastly, keep alive feature was moved into Connection class to reduce duplication :)

JamJar00 commented 6 years ago

Hey!

Firstly thanks for the pull request, you're code style is really good and you've kept everything nice an neat which is always good to see!

I'm interested in why you chose to manually send packets rather than setting the TCP keep alive socket option? Is there an advantage or limitation to one of them?

Also, you don't seem to have any code to handle when a keep alive packet is received so will it not be interpreted as a length header and corrupt the data when a keep alive packet is received?

Thanks for your hard work!

Jamie

brogowski commented 6 years ago

Hello.

I saw that ConnectionStatistics class is used with keep alive packets for UDP. So it makes sense to include TCP packets as well. Moreover, it was quite easy to implement since both TCP and UDP inherit from Connection class. Additionally, clients will have more flexibility because of tweakable time interval.

As for receiving, you've got me. I totally forgot to include that. I've copied test cases from UDP, changed them to use TCP and made them pass.

I will add receive code.

brogowski commented 6 years ago

Ok, I've added reading but I think it will need more work.

Current version is not that great - I'am assuming that we can treat packets with 0 length body as Keep Alives.

Additionally, I had to add new position in SendOption enum - KeepAlive.

@JamJar00 Do you think it's sufficient? Or we need to came up with solution similar to UdpSendOption as marker in header?

JamJar00 commented 6 years ago

Tricky. I personally feel that you should be able to send 0 length packets because a lot of people that have used DarkRift have used 0 length packets as simple triggers. Granted there's meta information sent in DarkRift so it's not truly 0 length but it would be good to ensure consistency between UDP and TCP in Hazel.

Perhaps a workaround would be to reserve header length 0xFFFFFFFF as a control flag and then specify some spec after that of control codes. My concern is that this is over complicating things though.

I'll have a closer look into the Socket keep alive flag, I think if we can use that then it's probably the simplest solution but I don't know.

Jamie