fix: mtt-1088 review. Safer handling of out-of-order or old messages #1091
Conversation
…g OOO messages or old messages
| } | ||
| else | ||
| { | ||
| m_ClientData[clientId].ReceivedSequenceMask = 0; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
But I'm just going to trust you forced this condition to happen and all was ok and approve
There was a problem hiding this comment.
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.
…nsform * develop: (26 commits) fix: client connected InvokeOnClientConnectedCallback with scene management disabled (#1123) fix: removed `public` class `NetcodeObserver` (MTT-1157) (#1122) feat: add NetworkMessageSent/Received metrics (#1112) feat: snapshot. MTU sizing option for Snapshot. MTT-1087 (#1111) Add metrics for transport bytes sent and received (#1104) fix: Missing end profiling sample (#1118) chore: support standalone mode for netcode runtimetests (#1115) feat: Change MetricNames for a more complex value type (#1109) feat: Track scene event metrics (#1089) style: whitespace fixes (#1117) feat: replace scene registration with scenes in build list (#1080) fix: mtt-857 GitHub issue 915 (#1099) fix: NetworkSceneManager exception when DontDestroyOnLoad NetworkObjects are being synchronized (#1090) feat: NetworkTransform Custom Editor Inspector UI (#1101) refactor: remove TempGlobalObjectIdHashOverride (#1105) fix: MTT-1124 Counters are now reported in sync with other metrics (#1096) refactor: convert using var statements to using var declarations (#1100) chore: updated all of the namespaces to match the tools package change (#1095) refactor!: remove network variable settings, network behaviour cleanup (#1097) fix: mtt-1088 review. Safer handling of out-of-order or old messages (#1091) ... # Conflicts: # com.unity.netcode.gameobjects/Prototyping/NetworkTransform.cs
…nity-Technologies#1091) * fix: Addressing latest review comments, making it safer when receiving OOO messages or old messages * fix: Addressing latest review comments, whitespace * fix: removing extraneous cast to ushort
Addressing comment form Matt that were missed in previous PR
overflow in left shift operands are now dealt with
reception of out-of-order packets results in not updating the mask (that's ok)
there was also a
+instead of an|which might have set the wrong bit if done twice (same message doubly acknowledged in different messages)