search

lidgren / lidgren-network-gen3

Lidgren Network Library
https://groups.google.com/forum/#!forum/lidgren-network-gen3
MIT License
1.19k stars 331 forks source link

Feature/Unit Test Improvement and Bugfixes #96

Closed RevoluPowered closed 6 years ago

RevoluPowered commented 6 years ago

This fixes the following bugs:

I have also added NUnit testing, this runs automatically from Travis and uses a Nuget package locally. This includes 3 new tests using NetServer and NetClient, I will be expanding these as I improve things.

My plan is to rate our coverage and test for memory leaks.

Platforms tested:

NET. Runtimes Tested:

Note Replaced socket.shutdown( Receive ) with socket.close(2) to free system resources properly across platforms, reported a bug to the mono guys regarding socket.shutdown as it's not behaving.

Travis has been updated to reflect these changes too.

All tests pass and have manually been checked.

RevoluPowered commented 6 years ago

@lidgren any chance you can review this?

lidgren commented 6 years ago

Hi! Thanks for taking an interest in the library. However...

I don't see how this "fixes" rebinding. When using the recommended 0 for m_configuration.Port (which lets the OS select a client port) - rebinding will not work after this change.

Calling Close() instead of Shutdown() will abruptly stop sending bytes, so any outgoing (lidgren) disconnect will not make it.

Your change NetPeer.cs#134 makes no logical difference.

Setting the network thread as a foreground thread will prevent the application from exiting; I believe a library should not make that call - if the application wants to wait for the networking to shut down properly it can do that manually; retaining the option to just shut down immediately.

There are also lots of other tidbits mixed into this request; making it difficult to take it wholesale.

RevoluPowered commented 6 years ago

Interesting, I have work today so I'll give a proper reply when I get back.

Thanks for the reply it's appreciated.

RevoluPowered commented 6 years ago

Sorry for getting back so late, been busy

"I don't see how this "fixes" rebinding. When using the recommended 0 for m_configuration.Port (which lets the OS select a client port) - rebinding will not work after this change."

I'll fix that.

"Calling Close() instead of Shutdown() will abruptly stop sending bytes, so any outgoing (lidgren) disconnect will not make it."

You're right, microsoft recommend that shutdown is called, then close is called after. I will see how to make this change, and make mono behave normally. Believe me I tried to explain that to the mono crowd already, this was a workaround for their lovely sigabrt bug which apparently is it behaving normally, which I just don't believe. Would appreciated if you could have a conversation with them about this.

I will fix the rebinding too, as that would definitely be a regression.

"Setting the network thread as a foreground thread will prevent the application from exiting; I believe a library should not make that call - if the application wants to wait for the networking to shut down properly it can do that manually; retaining the option to just shut down immediately."

OK, I'll make the thread background again, and leave the option in to 'GetNetworkThread()' if that's okay with you? I want to be able to join manually to shutdown as this wasn't possible to my knowledge beforehand.

Would you be happy if I make those changes, then re-submit this pull request for review?

I don't mind answering any other questions, also it might be worth us branching this into a different version as these changes may need tested by others before they're merged into master.

I'd prefer that too tbh.

RevoluPowered commented 6 years ago

In relation to the shutdown, close logic - the bug in question on osx: https://github.com/mono/mono/issues/7368

RevoluPowered commented 6 years ago

closing, no reply