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

"Feature": Bigger packets in network protocols #9099

Merged
merged 4 commits into from Apr 25, 2021

Conversation

@rubidium42
Copy link
Contributor

@rubidium42 rubidium42 commented Apr 24, 2021

Motivation / Problem

Some simple things in the network protocol are made much harder due to the hardcoded limit of 1460 bytes in a packet, even for cases where there is no real reason for such a low limit. This means that many things need to be split into several smaller packets, so allowing for larger packets requires fewer places where extra work is required to split communication in several smaller packets.

Description

Differentiate between UDP and TCP for the maximum packet size, though default to the original size due to backward compatibility reasons. This means that, by default, the sent packets are still limited to the old amount. However, if it is safe for the protocol to increase the packet size, i.e. you know for certain that the other side accepts the larger packets, then you can let that specific packet to be created with a larger limit.
For now the limit is set to 32 767 bytes, one byte fewer than 32 KiB. This is done to keep the future open for sending even larger packets, but that requires special encoding of packet sizes and makes everything more complicated. For now the sending of save games will therefor use 32 KiB packets.
Receiving of TCP will always accept 32 KiB packets regardless of the version at the other side, UDP is still limited to 1460 bytes.

As it stands with this patch the transfer of save games and requesting information from the content server use the 32 KiB packets. For the save games it is safe as you need the same version of the client, and the content server does no check on the packet size being <= 1460, so it already happily accepts the larger packets.

Limitations

The limit of packets is currently set to 32 767 bytes. The larger packet sizes are not implemented as there is no clear need for them yet. Limiting the packet size will allow you to prioritize other information channels (e.g. chat messages) instead of blocking the connection while downloading a large game or something.
UDP limit cannot be increased, mostly due to practical network limitations.
TCP limit for game cannot be increased for certain packets that can be sent in the early stages of joining. The first three pairs of PacketGameType as it stands now. The others could be increased, but as far as I can see it makes no sense for them as they are significantly smaller than 1460 bytes.

Future potential

TCP limit for admin could be increased, but that requires a protocol bump so we are certain that the client supports it and we would still need to maintain the old version with smaller packets as well. But it would allow to send larger responses.
TCP limit for receiving data from the content server requires a protocol change so the server knows it can send the client larger packets. The client will happily accept the packets.

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')
src/network/network_content.cpp Outdated Show resolved Hide resolved
@rubidium42 rubidium42 merged commit a3c9eca into OpenTTD:master Apr 25, 2021
12 checks passed
@rubidium42 rubidium42 deleted the bigger_packets branch Apr 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants