Fix problems with high load on RUDP

JamJar00 commented 6 years ago

There are currently problems with high load as demonstrated with BenchmarkNet. Hazel should not fall at any of the BenchmarkNet tests let alone at such an early test.

Vytek commented 6 years ago

Not so bad for 192 clients ;-)

JamJar00 commented 6 years ago

It's not too bad, but it still indicates a problem somewhere that could always trip up lower client counts in the wrong circumstances!

Vytek commented 6 years ago

How can we fix the problem? Using Profiler?

JamJar00 commented 6 years ago

Profiler won't help here really, just need to find the set of locks causing the deadlock (assuming it's a deadlock)

nxrighthere commented 6 years ago

Now the thing that really matters is just fixing the module crash that happens when more than 500 clients connected to the server.

System.InvalidOperationException: Could not send data as this Connection is not connected and is not connecting. Did you disconnect? in Hazel.Udp.UdpClientConnection.WriteBytesToConnection(Byte[] bytes) in Hazel.Udp.UdpConnection.SendAck(Byte byte1, Byte byte2) in Hazel.Udp.UdpConnection.ProcessReliableReceive(Byte[] bytes, Int32 offset) in Hazel.Udp.UdpConnection.ReliableMessageReceive(Byte[] buffer) in Hazel.Udp.UdpConnection.HandleReceive(Byte[] buffer) Unreliable: 30102 messages (1444896 bytes) in Hazel.Udp.UdpClientConnection.ReadCallback(IAsyncResult result)ble: 28828 messages (1383744 bytes) in System.Net.LazyAsyncResult.Complete(IntPtr userToken)nreliable: 28828 messages (1383744 bytes) in System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)ges (5641632 bytes), Unreliable: 116587 messages (5596176 bytes) in System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx) in System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state) in System.Net.ContextAwareResult.Complete(IntPtr userToken) in System.Net.LazyAsyncResult.ProtectedInvokeCallback(Object result, IntPtr userToken) in System.Net.Sockets.BaseOverlappedAsyncResult.CompletionPortCallback(UInt32 errorCode, UInt32 numBytes, NativeOverlapped nativeOverlapped) in System.Threading._IOCompletionCallback.PerformIOCompletionCallback(UInt32 errorCode, UInt32 numBytes, NativeOverlapped pOVERLAP)

xmedeko commented 6 years ago

How it's possible the Hazel has crashed while the DarkRift 2.1.0 has been doing OK? I though the DarkRift 2.1.0 is build on top of the Hazel.

JamJar00 commented 6 years ago

DarkRift 2 was originally going to be built on top but was changed during the development to a simpler bi-channel TCP/UDP setup to make it easier to maintain.

xmedeko commented 6 years ago

So, the README.md is wrong:

Hazel is going to be the basis of DarkRift 2 ...

Continued support as it's improved and maintained for DarkRift 2 ...

JamJar00 commented 6 years ago

Good spot, I forgot about that. Fixed.

xmedeko commented 6 years ago

Since building robust RUDP is not easy and RUDP may be implemented in different way (e.g. reliable with order, reliable without order, unreliable with order, ...). I suggest you to drop RUDP from Hazel and maintain bi-channel TCP/UDP communication like DarkRift2. This way, maybe the DarkRift2 may use Hazel again in the future?

JamJar00 commented 6 years ago

I don't think it would be fair to drop a connection method, there are people who use Hazel for it's RUDP and it would mean they had to change otherwise.

willardf commented 5 years ago

Okay, I feel like I'm resurrecting something here, but I wanted to gather some opinions.

Someone randomly suggested I run @nxrighthere's Benchmark.NET project against the fork of Hazel I've been developing for the last year or so. They had concerns with this issue showing up in those tests. Since I've been running my fork in production for 6+ months at 500+ users, I'm not surprised that I have fixed this issue.

unnamed

However I noticed Hazel is leading in bandwidth, and I'm sure my fork has improved the memory and CPU characteristics as well since I've removed several locks, allocations, and buffer copies.

The changes are deep enough and my focus diverged from @JamJar00's enough that I have considered rebranding entirely. But I've been developing strictly for my personal uses and I'm not really a "maintainer", I don't have a strong feel for if that is the right choice.

So uh, tl;dr. Is it possible I can get an external reading of my progress? The necessary changes to the benchmark are very simple. Should I rename my fork and part ways (with credit, of course) or is there room for an almost certainly breaking change PR?

Vytek commented 5 years ago

I vote for rebranding! Please add some simple example client-server. And Unity simple example, it is very usefull! Hazel next gen is back! Thank you for your help!

JohnBergman commented 5 years ago

What would the breaking change(s) be and how hard are they to compensate for in my code?

willardf commented 5 years ago

Well, if you hopped to my code verbatim:

I have removed fragmented RUDP entirely. Nothing about my code is incompatible, I didn't need it and it would have taken extra time to fix as I was plumbing MessageReader/Writers. A PR would probably take the time to readd this. Shouldn't be too hard, since it mostly piggy-backs on unfragmented RUDP.
TCP was removed and eventually readded from scratch, so I have no idea how compatible they are. This could cause some real problems in a PR picking one side or the other.
DataReceived events now have a MessageReader instead of raw bytes. These are pooled and need recycling. client.DataReceived += (connObj, evtObj) => { ... evtObj.Recycle(); }; is now client.DataReceived += (evtObj) => { ... evtObj.Message.Recycle(); }; This is the bulk of the library users' work. I strive to keep the MessageReader compliant with BinaryReader, but who knows how people are using the bytes currently. It puts additional pressure on library users to manage lifetimes and track down leaks, but also gives a few tools (counters and logging) for doing so.
- Users could hack around it with Buffer.Copy(target, 0, evtObj.Message.Bytes, 0, evt.Message.Length). But some people might also think less of those users.
Ideally, users use Send(MessageWriter) instead of SendBytes(byte[]) as well, although the latter does still exist and works well for fixed, unreliable messages.
Disconnecting was reworked pretty heavily, though users should only notice that it requires a string reason because I didn't like all the wrapped exception stuff. But I'm not 100% if there are semantic differences as well. (eg. Dispose doesn't call OnDisconnect, Disconnect calls Dispose.) There may be more/less/different exceptions than previous.

There may be other things. I haven't pulled up a diff or anything.

JamJar00 commented 5 years ago

Well, this is an interesting surprise! It's always nice to see Hazel still in use!

Frankly, the last real commit I made to Hazel was over a year ago so we might as well be honest with ourselves and call this a dead project. Whatever comes next is a revival.

Since you have breaking changes, and by the sounds of it a lot of divergence, I doubt there's any way a change such as that proposed could be a minor release. Therefore, I suggest there's only two practical paths to go down here:

You do a PR, we merge in your changes, perhaps with some prior editing for minor details, and then we release as Hazel 1.0.0; or,
you continue as is, with an optional rebrand if you want to (I'm not fussed if not!), and I will gladly point to your fork as the 'official'/'main' continuation of Hazel.

In my mind Hazel's been all but officially dead for years now, and I have no intention of continuing development on it myself. I've said personally that I would be happy to pass the project on if someone is able to give it the love, time and real application that it requires and I am unable to provide, and I'm happy that you've already shown that.

I would suggest, therefore, we do the latter option and I pass the reins on to you for the future. Your fork of Hazel (or whatever you decide to call it) becomes the 'official' continuation and I point to that on the current Hazel GitHub/NuGet to encourage adoption.

I thoroughly enjoyed working on Hazel and the unique challenges its implementation posed for me, and I really wish I had the time to invest in it to making it better. If you're happy to take over Hazel 'officially', I'd be more than happy to pass the baton to you 🙂

Jamie

willardf commented 5 years ago

Oh! If you're fine passing on the whole shebang, then I'd be honored! I'll pop into the DR2 discord and make myself known in case it'll smooth anything over.

JamJar00 commented 5 years ago

Glad you're happy to take over! I'll add notices to GitHub/Nuget and make announcements when I have a bit more time later this week if that's ok with you.

Feel free to hang around in the Discord, people are most welcoming there, and you're more than welcome to send future questions over to me if you think I'll be able to help!

I'm honestly delighted that Hazel isn't just another abandoned project!

JamJar00 commented 5 years ago

Apologies I haven't done this yet, I haven't had much time! I haven't forgotten though! 🙂

Do you have a Unity forum handle I can tag in posts?

willardf commented 5 years ago

No worries! I have plenty of my own business to mind. Me and my one post can be found here: https://forum.unity.com/members/fortebass.988271/

dstovell-kabam commented 5 years ago

Is there PR available for these changes?

I'm getting a lot of disconnects with high packet load. My stress test goes crazy with resends at around 500+ packets per second.

willardf commented 5 years ago

There is no PR for this. Rather my fork has overtaken Jamie's. There are some usage differences, but they should be relatively easy to navigate. https://github.com/willardf/Hazel-Networking

I also have a big usage example repo, but I recognize that I need to put some simpler usage scenarios in the Hazel wiki or something. https://github.com/willardf/Hazel-Examples

On Fri, Jun 21, 2019 at 12:35 PM Dan Stovell - dstovell < notifications@github.com> wrote:

Is there PR available for these changes?

I'm getting a lot of disconnects with high packet load. My stress test goes crazy with resends at around 500+ packets per second.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/DarkRiftNetworking/Hazel-Networking/issues/12?email_source=notifications&email_token=AAJTL6SSUOUNUFGFW7N46ODP3UUOLA5CNFSM4EPSLDH2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYJMUWA#issuecomment-504547928, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJTL6QIVHAP7LLDIHP5ZLTP3UUOLANCNFSM4EPSLDHQ .

dstovell-kabam commented 5 years ago

I'm actually already looking at what I need to change to integrate. Thank you!

dstovell-kabam commented 5 years ago

@willardf tried out your fork and it seems to work better with high packet counts. I'm looking at hooking up DTLS and I'm wondering if you have an "nice to knows" about using it with Hazel.

Sorry for the reply here, looks like the issues tab is not available on your fork.

willardf commented 5 years ago

Hey @dstovell-kabam. Wow, sorry I completely managed to miss your message for months. I opened up the issues tab so you can recreate something there. But in general, I have no experience with DTLS or secure UDP, so I'd be very interested in seeing what that looks like.

Given my complete lack of knowledge, I doubt there should be any gotchas around it. Just try to preserve the MessageReader/Writer interactions with the UdpConnection subclasses. Putting normal data in and getting normal data out is the key to keeping things happy in Hazel.

DarkRiftNetworking / Hazel-Networking

Fix problems with high load on RUDP #12