Skip to content
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.

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

NetworkServer SendToAll does not check size of message before sending #3710

Closed
James-Frowen opened this issue Jan 1, 2024 · 1 comment
Closed
Assignees
Labels
bug Something isn't working

Comments

@James-Frowen
Copy link
Contributor

Describe the bug
When NetworkServer.SendToAll is called it does not check the size before calling conn.Send(ArraySegment...). This can cause large message to be added to batches which will then result in unhelpful errors:

NetworkConnection.ValidatePacketSize: cannot send packet larger than 64350 bytes, was 5746485 bytes
UnityEngine.Debug:LogError (object)
Mirror.NetworkConnection:ValidatePacketSize (System.ArraySegment`1<byte>,int) (at Assets/Plugins/Mirror/Core/NetworkConnection.cs:104)
Mirror.NetworkConnection:Update () (at Assets/Plugins/Mirror/Core/NetworkConnection.cs:190)
Mirror.NetworkConnectionToClient:Update () (at Assets/Plugins/Mirror/Core/NetworkConnectionToClient.cs:181)
Mirror.NetworkServer:Broadcast () (at Assets/Plugins/Mirror/Core/NetworkServer.cs:1703)
Mirror.NetworkServer:NetworkLateUpdate () (at Assets/Plugins/Mirror/Core/NetworkServer.cs:1777)
Mirror.NetworkLoop:NetworkLateUpdate () (at Assets/Plugins/Mirror/Core/NetworkLoop.cs:206)

[IMPORTANT] How can we reproduce the issue, step by step:

  • call NetworkServer.SendToAll<MyMessage>(...) with a message that is bigger than transport can send

Expected behavior
SendToAll should check size (by calling ValidatePacketSize ?). and log error with the stack that will tell user what message is too big, for example:

...
Mirror.NetworkServer:SendToAll<MiscInfoMessage> (MiscInfoMessage,int,bool) (at Assets/Plugins/Mirror/Core/NetworkServer.cs:453)
NetworkCommands:UserCode_CmdSetLevelInformation__LevelInformationDataSO (LevelInformationDataSO) (at Assets/Scripts/Utility/NetworkCommands.cs:571)
...

This will then allow user to track down what is sending it

Desktop (please complete the following information):

  • Mirror branch: v78.3.0
    Looks like it would be a problem in v86.14.0, but I havn't tested that

Additional context

source:

{
// pack message only once
NetworkMessages.Pack(message, writer);
ArraySegment<byte> segment = writer.ToArraySegment();
// filter and then send to all internet connections at once
// -> makes code more complicated, but is HIGHLY worth it to
// avoid allocations, allow for multicast, etc.
int count = 0;
foreach (NetworkConnectionToClient conn in connections.Values)
{
if (sendToReadyOnly && !conn.isReady)
continue;
count++;
conn.Send(segment, channelId);
}
NetworkDiagnostics.OnSend(message, channelId, segment.Count, count);
}

@MrGadget1024 MrGadget1024 added the bug Something isn't working label Jan 2, 2024
@miwarnec
Copy link
Collaborator

miwarnec commented Jan 2, 2024

2024-01-02 - 13-13-30@2x fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants