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

Change: no longer use UDP when entering the lobby of a server #9114

Merged
merged 5 commits into from Apr 27, 2021

Conversation

TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Apr 26, 2021

Motivation / Problem

With STUN in mind, it is a lot easier if we have a single protocol to talk between client and server. This was mostly true, except for a part in the lobby window.

Most likely to avoid code-duplication (random guess honestly), opening the lobby does two requests: one via UDP to get the latest game-info, and one via TCP to get the latest company-info. This PR sets out to change that both to TCP.

Accidentally, this solves a rare edge-case, where someone create a new company on a server, TCP reacts with the company-list, but the UDP packet is lost. In this case, the new company is not shown. Only until the UDP packet is received, it will be shown correctly. By using TCP here over UDP, we ensure this isn't possible.

Description

The first commit moves a lot of code related to NetworkGameInfo to a single place, so adding GameInfo to TCP (next to UDP) doesn't cause code duplication.

The second commit adds a packet to the TCP to exchange GameInfo over TCP too.


Sadly, I wasn't a very smart cookie when I wrote the network protocol, and over TCP we do not handshake a protocol-version before we initiate conversation. This means that there are potential (future) issues with this PR. Currently this isn't an issue, but just to walk through the situations:

New client + new server:

  • MasterServer tells the servers online, via UDP these are queried. When pressing Join, the Lobby window does everything via TCP.
  • Via "Add Server" someone manually adds a server. Via UDP it is queried. When pressing Join, the Lobby window does everything via TCP.

Old client + new server / new client + old server:

  • MasterServer tells the servers online, via UDP these are queried. Versions are incompatible, so lobby cannot be opened.
  • Via "Add Server" someone manually adds a server. Via UDP it is queried. Versions are incompatible, so lobby cannot be opened.

Using -n to join a server bypasses lobby, and as such works fine too (well, it hits revision mismatch if that is the case, ofc)

Backport

Currently "Add Server" still uses UDP to query the server. A future PR will change this to also use TCP here (only for manual servers and manual refresh). This does mean that adding older servers will result in a protocol error, as older server don't know this new packet. In such future PR, the client will pick up on this and show a nice "Server is too old" text.

It would help to backport this to 1.11. It would mean that when 1.12 releases, the 1.11.N can be added manually and show the game info (and that the version is incompatible). This would mean for most of our servers there isn't a real problem, and it is only an issue if people add really old servers manually.
This is also the reason that this future PR is not included in this PR.

Additionally, the last two commits from this PR don't have to be backported. It makes use of this new TCP packet; without it nothing breaks, and means there is no functional difference after backporting. But it is better to play it safe, so do not backport the last two commits please.

Additionally, if this is backported, so should #9129, as it fixes packet order.

Limitations

None, this should be fully backports compatible. The GUI prevents opening the lobby for old servers, and UDP is used for anything before that.

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 master-tcp-server-info branch 2 times, most recently from 8a00497 to 7369eb5 Compare April 26, 2021 14:58
@TrueBrain TrueBrain changed the title Change: don't require UDP when opening the lobby of a server Change: no longer use UDP when entering the lobby of a server Apr 26, 2021
@TrueBrain TrueBrain force-pushed the master-tcp-server-info branch 5 times, most recently from 57479e3 to 8fb4d1c Compare April 26, 2021 15:27
src/network/core/game_info.cpp Outdated Show resolved Hide resolved
src/network/network_client.cpp Outdated Show resolved Hide resolved
src/network/core/game_info.cpp Outdated Show resolved Hide resolved
src/network/core/game_info.cpp Outdated Show resolved Hide resolved
src/network/network_udp.cpp Show resolved Hide resolved
src/network/network_udp.cpp Outdated Show resolved Hide resolved
src/network/core/game_info.h Outdated Show resolved Hide resolved
src/network/core/game_info.h Show resolved Hide resolved
@TrueBrain TrueBrain force-pushed the master-tcp-server-info branch 2 times, most recently from 219457e to c95c6f1 Compare April 26, 2021 15:56
@TrueBrain TrueBrain added the backport requested This PR should be backport to current release (RC / stable) label Apr 26, 2021
@TrueBrain TrueBrain force-pushed the master-tcp-server-info branch 2 times, most recently from af20618 to dd30200 Compare April 26, 2021 18:09
src/network/core/game_info.cpp Outdated Show resolved Hide resolved
src/network/core/game_info.cpp Outdated Show resolved Hide resolved
rubidium42
rubidium42 previously approved these changes Apr 27, 2021
src/network/core/game_info.cpp Outdated Show resolved Hide resolved
It currently was a bit scattered over the place. Part of
NetworkGameInfo is also the GRF Identifiers that goes with it.
Later commits use this function in other places too.
The lobby of a server requested some parts via UDP and some via
TCP. This is strictly seen fine, but for future extensions it
is a lot easier if just one protocol is used.
@TrueBrain TrueBrain merged commit 31f1db2 into OpenTTD:master Apr 27, 2021
@TrueBrain TrueBrain deleted the master-tcp-server-info branch April 27, 2021 18:18
@LordAro LordAro added backported This PR is backported to a current release (RC / stable) and removed backport requested This PR should be backport to current release (RC / stable) labels May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported This PR is backported to a current release (RC / stable)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants