Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make networking async #17664

Open
lifecoder-phoenix opened this issue Feb 10, 2020 · 4 comments
Open

Make networking async #17664

lifecoder-phoenix opened this issue Feb 10, 2020 · 4 comments

Comments

@lifecoder-phoenix
Copy link

@lifecoder-phoenix lifecoder-phoenix commented Feb 10, 2020

Motivation

I read an issue stating that a lot of observers causes slowdowns(#17656), so I dived into the code and found that the networking part is done in a huge while loop(on a different thread) I'd like to propose the usage of asynchrous socket usage to solve the issues the loop creates

Proposed solution

I suggest(and am currently writing a PoC) to use Sockets instead of TcpListener and use async calls like BeginAccept, EndAccept, BeginReceive, EndReceive to not have it all going in the same thread in a huge while loop. It would definitely make the server faster and able to handle more connections.

Side effects

Since I'm new to the codebase I can't predict any side effects. As far as I can see it should work out of the box without negative side effects.

Alternatives

Keep the while loop, but god, why.

@abcdefg30

This comment has been minimized.

Copy link
Member

@abcdefg30 abcdefg30 commented Feb 10, 2020

@jrb0001's #17571 is currently changing parts of the networking code. I fear there will be conflicts?

@lifecoder-phoenix

This comment has been minimized.

Copy link
Author

@lifecoder-phoenix lifecoder-phoenix commented Feb 10, 2020

Thanks for pointing that out, didn't see that pull. It would conflict only in the Server.cs file though, since he wants to make a list of TcpListeners whilst I think it would be more efficient to just use async. But it will need some discussing with him then

@pchote

This comment has been minimized.

Copy link
Member

@pchote pchote commented Feb 10, 2020

OpenRA (like most RTS games) uses a deterministic lockstep model for networking, which is inherently synchronous. Making the sockets async won't help because the game clients still need to block on orders/sync data from all clients before simulating the next tick.

The real fix for spectator lag is to change the client handling so that spectators aren't expected to send non-immediate orders, and to change the main tick loop timings to allow spectators to tick as fast as possible to catch up if they end up behind.

@jrb0001

This comment has been minimized.

Copy link
Contributor

@jrb0001 jrb0001 commented Feb 22, 2020

@lifecoder-phoenix The fix for the spectator lag is to "disconnect" all spectators from the game so the player clients no longer wait for them. My server implementation already does that and it works (but can corrupt replays due to workarounds in the replay loading code).

If you want to rewrite the networking code for async, I strongly recommend using async/await/Task based on TcpListener over using the socket directly. TLS with ALPN (currently blocked by the migration to netstandard2.1) can only be done that way: SslStream.AuthenticateAsClientAsync(SslClientAuthenticationOptions sslClientAuthenticationOptions, CancellationToken cancellationToken) and SslStream.AuthenticateAsServerAsync(SslServerAuthenticationOptions sslServerAuthenticationOptions, CancellationToken cancellationToken) are the only ones that accept ALPN configuration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.