fix: Sends failing on UTP adapter when queue is full #1317
Merged
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
Or close to full. There were two issues here. First, the payload capacity of the fragmentation pipeline didn't account for the entire size of the packet. Unfortunately, payload capacity in the context of the fragmentation pipeline is a bit of a misnomer since it also includes headers and other such overhead. We were setting it to the size of a full send queue, and so when you added UTP headers, we ended up with a packet too large for the pipeline. Bumping up the value a bit solves this issue. Second, when sending a single message that was within 4 bytes of the send queue capacity, we'd silently fail to send it. The reason was we didn't account for the extra overhead taken by the data length in the send queue (which requires 4 bytes). Thus the send code would think the message could fit in the queue when it couldn't. Fixing this simply required accounting for the 4 extra bytes in the send logic. Two tests were also added to test sends when the send queue is full (either from one big send or from multiple smaller ones in a row). Both tests fail without the fixes in the adapter.
This one depends on a fix in UTP that's still being reviewed, though.
|
I've updated the fix to avoid having to use extra bytes in the fragmentation pipeline capacity to get a full send queue to work correctly. It was hacky and was actually papering over an actual issue in UTP. The fix for the UTP issue is in review (tentatively scheduled for 1.0.0-pre.7). If we're going to merge this PR before we depend on that new version, we'll want to disable the new tests. Note that this fix is still just adding extra bytes to the fragmentation pipeline payload capacity, but now the extra bytes are only there to account for overhead caused by the UTP adapter itself (by the send queue, to be more specific). And it's also exactly how many extra bytes are required so it doesn't feel like some magic number. |
Cosmin-B
suggested changes
Oct 20, 2021
Events can't be larger than m_SendQueueBatchSize. UTP will drop them and log an error in the current configuration. Reword the warning so that we don't imply that it's been sent, only that we're trying to send it. If it fails, there will be an error in the log following it, making it clear to the user what happened.
Cosmin-B
approved these changes
Oct 20, 2021
andrews-unity
approved these changes
Oct 21, 2021
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.
Or close to full. There were two issues here.
First, the payload capacity of the fragmentation pipeline didn't account for the entire size of the packet. Unfortunately, payload capacity in the context of the fragmentation pipeline is a bit of a misnomer since it also includes headers and other such overhead. We were setting it to the size of a full send queue, and so when you added UTP headers, we ended up with a packet too large for the pipeline. Bumping up the value a bit solves this issue.
Second, when sending a single message that was within 4 bytes of the send queue capacity, we'd silently fail to send it. The reason was we didn't account for the extra overhead taken by the data length in the send queue (which requires 4 bytes). Thus the send code would think the message could fit in the queue when it couldn't. Fixing this simply required accounting for the 4 extra bytes in the send logic.
Two tests were also added to test sends when the send queue is full (either from one big send or from multiple smaller ones in a row). Both tests fail without the fixes in the adapter.
Changelog
com.unity.netcode.adapter.utp
Testing and Documentation