Skip to content

feat: Add methods for setting max message size limits (i.e., MTU negotiation) [MTT-6214] #2530

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

Merged
merged 7 commits into from
May 24, 2023

Conversation

ShadauxCat
Copy link
Collaborator

@ShadauxCat ShadauxCat commented Apr 26, 2023

Adds new APIs. Do not merge until after the release/1.4.1 branch has been created.

resolves #2431

Changelog

  • Added: Message size limits (max single message and max fragmented message) can now be set using NetworkManager.SetMaxSingleMessageSize() and NetworkManager.SetMaxFragmentedMessageSize() for transports that don't work with the default values

Testing and Documentation

  • Includes unit tests.
  • Includes documentation for previously-undocumented public API entry points.

@ShadauxCat ShadauxCat requested a review from a team as a code owner April 26, 2023 19:23
Comment on lines 583 to 596
/// <summary>
/// Sets the maximum size of a single non-fragmented message (or message batch) passed through the transport.
/// This should represent the transport's MTU size, minus any transport-level overhead.
/// </summary>
/// <param name="size"></param>
public void SetMaxSingleMessageSize(int size) => MessageManager.SetMaxSingleMessageSize(size);

/// <summary>
/// Sets the maximum size of a message (or message batch) passed through the transport with the ReliableFragmented delivery.
/// Warning: setting this value too low may result in the SDK becoming non-functional with projects that have a large number of NetworkBehaviours or NetworkVariables, as the SDK relies on the transport's ability to fragment some messages when they grow beyond the MTU size.
/// </summary>
/// <param name="size"></param>
public void SetMaxFragmentedMessageSize(int size) => MessageManager.SetMaxFragmentedMessageSize(size);

Copy link
Contributor

@0xFA11 0xFA11 Apr 27, 2023

Choose a reason for hiding this comment

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

2 things:

  1. I'd suggest get; set; property here (so that users could get the value without setting it)
  2. I'd suggest naming these APIs as int MaximumTransmissionUnitSize { get; set; } & int MaximumFragmentedMessageSize { get; set; } because it looks like they boil down to those two concepts.

@ShadauxCat ShadauxCat marked this pull request as draft April 28, 2023 20:58
@ShadauxCat ShadauxCat marked this pull request as ready for review May 11, 2023 19:59
ShadauxCat and others added 2 commits May 23, 2023 14:49
Copy link
Collaborator

@NoelStephensUnity NoelStephensUnity left a comment

Choose a reason for hiding this comment

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

LGTM!

@ShadauxCat ShadauxCat enabled auto-merge (squash) May 24, 2023 18:02
@ShadauxCat ShadauxCat merged commit 99a1b94 into develop May 24, 2023
@ShadauxCat ShadauxCat deleted the feat/allow_setting_message_size_limits branch May 24, 2023 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Static MTU value is too high, and doesn't work with other transports
3 participants