Skip to content

fix: mtt-1088 review. Safer handling of out-of-order or old messages #1091

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Aug 26, 2021
Merged

fix: mtt-1088 review. Safer handling of out-of-order or old messages #1091

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
10492fe
fix: Addressing latest review comments, making it safer when receivin…
jeffreyrainy Aug 25, 2021
9c7d425
fix: Addressing latest review comments, whitespace
jeffreyrainy Aug 25, 2021
2fff30a
fix: removing extraneous cast to ushort
jeffreyrainy Aug 26, 2021
92f94d8
Merge remote-tracking branch 'origin/develop' into fix/MTT-1088-review2
jeffreyrainy Aug 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 34 additions & 15 deletions com.unity.netcode.gameobjects/Runtime/Core/SnapshotSystem.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -959,26 +959,45 @@ internal void ReadSnapshot(ulong clientId, Stream snapshotStream)
snapshotTick = reader.ReadInt32Packed();
var sequence = reader.ReadUInt16();

// todo: check we didn't miss any and deal with gaps

if (m_ClientData[clientId].ReceivedSequenceMask != 0)
if (sequence >= m_ClientData[clientId].LastReceivedSequence)
{
// since each bit in ReceivedSequenceMask is relative to the last received sequence
// we need to shift all the bits by the difference in sequence
m_ClientData[clientId].ReceivedSequenceMask <<=
(sequence - m_ClientData[clientId].LastReceivedSequence);
}
if (m_ClientData[clientId].ReceivedSequenceMask != 0)
{
// since each bit in ReceivedSequenceMask is relative to the last received sequence
// we need to shift all the bits by the difference in sequence
var shift = sequence - m_ClientData[clientId].LastReceivedSequence;
if (shift < sizeof(ushort) * 8)
{
m_ClientData[clientId].ReceivedSequenceMask <<= shift;
}
else
{
m_ClientData[clientId].ReceivedSequenceMask = 0;
Copy link
Contributor

@mattwalsh-unity mattwalsh-unity Aug 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume c# treats shift as an unsigned here? So then if there's an overflow we just set the mask back to 'nothing ackd'. I guess in that sense the cast or an explicit ushort type specifier might actually make sense

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I'm just going to trust you forced this condition to happen and all was ok and approve

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're are in if (sequence >= m_ClientData[clientId].LastReceivedSequence) and not in if (shift < sizeof(ushort) * 8)
and shift is sequence - m_ClientData[clientId].LastReceivedSequence;

So, not the most obvious, but shift is positive, when we use it.
If we take the else branch, it means shift was positive and above the size, so we meant to completely shift out the current content. = 0 does that, but <<= would not.

For testing, the best I have is letting it run at 50 spawn/s with 50% packet loss on both sending and receiving. It's not bullet proof, but I think it's ok for now.

}
}

if (m_ClientData[clientId].LastReceivedSequence != 0)
if (m_ClientData[clientId].LastReceivedSequence != 0)
{
// because the bit we're adding for the previous ReceivedSequenceMask
// was implicit, it needs to be shift by one less
var shift = sequence - 1 - m_ClientData[clientId].LastReceivedSequence;
if (shift < sizeof(ushort) * 8)
{
m_ClientData[clientId].ReceivedSequenceMask |= (ushort)(1 << shift);
}
}

m_ClientData[clientId].LastReceivedSequence = sequence;
}
else
{
// because the bit we're adding for the previous ReceivedSequenceMask
// was implicit, it needs to be shift by one less
m_ClientData[clientId].ReceivedSequenceMask +=
(ushort)(1 << (ushort)((sequence - 1) - m_ClientData[clientId].LastReceivedSequence));
// todo: Missing: dealing with out-of-order message acknowledgments
// we should set m_ClientData[clientId].ReceivedSequenceMask accordingly
// testing this will require a way to reorder SnapshotMessages, which we lack at the moment
//
// without this, we incur extra retransmit, not a catastrophic failure
}

m_ClientData[clientId].LastReceivedSequence = sequence;

var sentinel = reader.ReadUInt16();
if (sentinel != SentinelBefore)
{
Expand Down