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

Replace master-server (UDP-based) with Game Coordinator (TCP-based) #9411

Merged
merged 8 commits into from Jul 10, 2021

Conversation

TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jul 3, 2021

Motivation / Problem

This is a part of #9017.

Currently OpenTTD communicates over UDP with the master-server to register itself, and the master-server uses UDP to polls the server for game-state. This has several disadvantages, as we noticed over the years:

  • The users that do manage to setup a port-forwarding, often only manage to do this for TCP. Doing it for UDP too, is often either difficult in routers, or just not something they understand. As such, people have a lot of trouble setting up portforwarding in such a way the master-server can see the server. But to make matters worse, without UDP port-forwarding, people can join a server via the direct IP. This is frustrating to users.
  • UDP is, by definition, unreliable. So we have a lot of code to retry several times and trying to recover.
  • UDP has a hard MTU-size, as fragmentation is impossible. We try to be on the safe-side with the MTU we picked, but there are known scenarios where you cannot host OpenTTD because the connection doesn't allow such large UDP packets to travel through it. (mostly GRE links etc) Explaining this to users is very very difficult.
  • Feature: play easier together with friends from behind home routers #9017 works towards adding STUN support. For this we need a persistent connection between something on the openttd.org infra and the server, so we can push messages to the server of which we know they arrive safely. In theory we could use UDP for this, and with some smart keep-alive etc we can keep a bi-direction connection alive .. but it is tricky. More-over, most infrastructure, also on our hosting side, is more aimed at TCP connections than UDP connections, and several issues are to be expected.
  • Pretty sure I miss a few arguments here.

In total, it became pretty clear we should just stop using UDP, and switch completely to TCP. Well, I say completely .. local LAN discovery is and always will be UDP based. That is just how multicasts work ;)

To be clear, this doesn't mean OpenTTD will always be using TCP. It is surely an option to use some library like https://github.com/ValveSoftware/GameNetworkingSockets that does resilient connections over UDP. Either way, we first have to change to a single way of doing stuff, before we can think about changing it all to something else. And this is far out of scope of this PR.

Description

This removes all master-server communication and all UDP-related code from OpenTTD, and replaces it with a TCP-based Game Coordinator code.
Again, the local LAN discovery still uses UDP and remains.

Why the change of name? To keep it clear that this is a completely different component. And in the future the Game Coordinator will be tasked with many other tasks, like join-keys, STUN, TURN, etc. It much more takes on the role of a coordinator, and no longer "just" lists the servers.
In the background, master-server and Game Coordinator communicate with each other, so the server-listing will be a combination of both for 1.12+. 1.11 will only see 1.11 and older servers.

The way it works is like this (and yes, this is documented in network_coordinator.cpp, just to beat LordAro to the punch :P):

/**
 * Game Coordinator communication.
 *
 * For servers:
 *  - Server sends CLIENT_REGISTER.
 *  - GC probes server to check if it can directly connect.
 *  - GC sends SERVER_REGISTER_ACK with type of connection.
 *  - Server sends every 30 seconds CLIENT_UPDATE.
 *
 * For clients (listing):
 *  - Client sends CLIENT_LISTING.
 *  - GC returns the full list of public servers via SERVER_LISTING (multiple packets).
 */

Basically this means as soon as the client clicks "Search Internet", it gets the full list of servers, including their details. It no longer has to send a flood of UDP packets to all servers.

Private is renamed to Local, as there is no such thing as Private .. a server is either accessible locally (or by the IP if you happen to know that), or publicly listed. This change makes the later addition of Invite Only a lot more sensible to the user.

Future PRs will extend this functionality with invite-codes, STUN, TURN, ... But, baby-steps.

Limitations

Currently there are a few rough edged, but further PRs will address those. For example:

  • The CLIENT_UPDATE sends all the NewGRF data every time. But as they can never change, this is a waste of bandwidth.
  • Currently servers can only register via either IPv4 or IPv6, not both. This is because future PR will use the STUN requests to discover the IPs. I could strictly seen already add this in this PR, but that would increase the change drastically. And solving it differently is rather wasting my time a lot, as I have to create something I have to remove a few PRs later.
  • Currently servers registering via IPv6 will be told that they do not have their firewall open. This might not be true, but the Game Coordinator can't query IPv6 yet.
  • Currently servers using 1.11.2 or earlier are not showing up in the server listing. This will be addressed before 1.12.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@TrueBrain TrueBrain force-pushed the game-coordinator branch 2 times, most recently from 8905bd6 to b6aa8dd Compare July 3, 2021 19:45
src/network/core/tcp_coordinator.cpp Outdated Show resolved Hide resolved
src/network/core/tcp_coordinator.cpp Outdated Show resolved Hide resolved
src/network/core/tcp_coordinator.h Outdated Show resolved Hide resolved
src/network/network_coordinator.cpp Outdated Show resolved Hide resolved
src/network/network_coordinator.cpp Outdated Show resolved Hide resolved
src/network/network_coordinator.h Show resolved Hide resolved
src/network/core/tcp_coordinator.h Outdated Show resolved Hide resolved
src/network/core/tcp_coordinator.h Outdated Show resolved Hide resolved
src/network/network_gui.cpp Outdated Show resolved Hide resolved
src/network/core/config.h Outdated Show resolved Hide resolved
@embeddedt
Copy link
Contributor

Another argument for dropping UDP support: ngrok (and several other reverse-tunneling services) only work with TCP, so this is a very welcome change. 👍

@TrueBrain TrueBrain marked this pull request as ready for review July 10, 2021 16:00
@TrueBrain
Copy link
Member Author

I think this is ready. Game Coordinator is deployed, and talks (two-way) with the Master Server. So anything listed in the Master Server shows up via the Game Coordinator, and the other way around.

Of course you cannot join those servers that are exchanged, as they for sure are of a different revision, but that is not really the point here ;)

It could use some testing of some people, just registering their server, see if they can find it, etc etc. But .. I think this is ready :)

As we now use the Game Coordinator for announcements, there is no
longer a need to use the Master Server for this.
This removes all UDP from the game except for a local broadcast
to find LAN games.

So long Master Server, and tnx for all the fish!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants