fix: scene switch stream corruption #504
Closed
Conversation
This file contains 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
…s to fix a rare race condition bug.
LukeStampfli
requested review from
NoelStephensUnity,
TwoTenPvP and
mattwalsh-unity
| @@ -1030,6 +1030,10 @@ private void HandleRawTransportPoll(NetEventType eventType, ulong clientId, Chan | |||
| } | |||
|
|
|||
| private readonly BitStream m_InputStreamWrapper = new BitStream(new byte[0]); | |||
| bool m_InputStreamWrapperUsed; | |||
| // The fallback wrapper is used in case we have to handle incoming data but the InputStreamWrapper is already being used. | |||
mattwalsh-unity
approved these changes
Feb 26, 2021
mattwalsh-unity
changed the title
…tream-corruption # Conflicts: # com.unity.multiplayer.mlapi/Runtime/Core/NetworkingManager.cs
# Conflicts: # com.unity.multiplayer.mlapi/Runtime/Core/NetworkManager.cs
| @@ -918,6 +918,16 @@ private void HandleRawTransportPoll(NetworkEvent networkEvent, ulong clientId, N | |||
| } | |||
|
|
|||
| private readonly NetworkBuffer m_InputBufferWrapper = new NetworkBuffer(new byte[0]); | |||
| bool m_InputBufferWrapperUsed; | |||
There was a problem hiding this comment.
Suggested change
| bool m_InputBufferWrapperUsed; | |
| private bool m_InputBufferWrapperUsed; |
|
@LukeStampfli — we recently merged release back into develop and also it's been a while since we last tested this. I wonder if you could merge latest develop into this PR then also implement a simple test that exposes the underlying issue consistently so that we could make sure that this fix is indeed fixes the problem (we should have some form of a test going with this regardless IMHO) 🙂 |
|
I think there is a better solution to this (improving the underlying code rather then hotfixing this). Will start a discussion once I get time. |
|
Sounds good and will be looking forward to hearing more about the better
solution!
…On Thu, Apr 1, 2021 at 7:55 AM Luke Stampfli ***@***.***> wrote:
I think there is a better solution to this (improving the underlying code
rather then hotfixing this). Will start a discussion once I get time.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#504 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AROMJ5OSGKMCCC5OBKROLNTTGRULNANCNFSM4YIFIADA>
.
|
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 is a fix for a rare race condition which causes an exception and screws over the entire simulation on the client side.
The Bug
New client connects to server.
Server send a packet
(p1)containing spawn information of all objects(o1, o2, ....)to new client (reliable sequenced)Server code invokes RPC on existing object (o1) shortly after creating a reliable unsequenced message
(p2)Client receives
p2first and stores it as a buffered message.Client receives
p1sets the buffer of m_InputStreamWrapper to received message and starts spawning objecto1. While spawning objecto1it checks for buffered messages and findsp2.Client applies
p2to newly spawned object. It does so by calling HandleIncommingData with the buffered message. This overides the buffer of m_InputStreamWrapper with the content ofp2.Client continues spawning objects
(o2)but m_InputStreamWrapper now contains the wrong buffer of the messagep2instead of. Clients reads PrefabHash and receives a corrupted value (most of the time ulong.maxvalue) and throws.Standard RPC fixes this to some extend because it uses the reliable sequenced channel by default (I think). I still think we should fix this because if we ever end up sending something unsequenced we will get these weird stream corruptions again.
This fix works for the current implementation of MLAPI and as long as we don't end up nesting HandleIncommingData more then twice. If that's the case we will need to switch a more sophisticated pool. PooledBitStream cannot be used for this because we override the target array of the stream which is not supported.