fix: Address packet overflow issues in the UTP adapter #1403
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It doesn't mean anything now. Anything sent on fragmented pipelines will be broken up at 1400 bytes boundaries, and everything sent on a non-fragmented pipeline is limited to a maximum size of 1400 bytes (including overhead from the underlying protocols). Allowing to set that limit can only lead to confusion among users.
This is not the cleanest of solution, but until we figure out something better in UTP, this can at least unblock Boss Room. The core issue being fixed here is that we'd allow sending payloads up to the MTU in size on a non-fragmented pipeline. But that's incorrect since UTP must add its own overhead to the payload. If sending a payload close to the MTU in size, the overhead would bring the total packet size over the limit of the MTU. Since UTP lacks a good way of figuring out what's the actual maximum payload size that can be sent, for now we patch over the issue by sending everything on a fragmented pipeline. The exception is the unreliable messages. The fragmentation pipeline doesn't cleanly handle having multiple fragmented payloads in flight (and possibly arriving out of order). The reliable pipeline stage fixes that, but for unreliable traffic it's better not to fragment it.
andrews-unity
approved these changes
Nov 8, 2021
Which is 2000 bytes. We won't need to hardcode that default value when the new NetworkSettings API lands. So having a magical number floating like that in the code is a temporary thing.
andrews-unity
added
stat:backported-release-1.0.0
and removed
type:backport-release-1.0.0
labels
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces two changes to the UTP adapter.
First, it removes the possibility of setting the maximum packet size. That setting didn't mean anything since UTP always considers the MTU (1400 bytes on most platforms) to be the maximum packet size on non-fragmented pipelines. Being able to set that value could also create confusion, since one might think that sending payloads of the configured size would work. That's (counter-intuitively) not the case, since UTP needs to add its own overhead to the payload, and the configured value would be for the entire packet (payload + overhead).
Second, it sends all reliable traffic on a fragmented pipeline. Because of the above issue (UTP needing to add its own overhead to payloads), we'd have situation where we'd try sending a payload close to the MTU in size over a non-fragmented pipeline. With the UTP overhead, that would bump it above the MTU which would cause a lot of issues.
Admittedly, that second change is not a very clean one. Ideally we'd only send on fragmented pipelines payload that actually need to be fragmented. That will probably require changes in UTP, though, so in the meantime we propose this fix.
This addresses MTT-1685 (and possibly MTT-1681) if I understand its scope correctly.
Changelog
com.unity.netcode.adapter.utp
Testing and Documentation