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

Add fragmentation support #36

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

Conversation

js6pak
Copy link
Contributor

@js6pak js6pak commented Dec 16, 2021

Fragments all reliable packets with sizes over the connection MTU, trying to send an unreliable packet that is too big simply throws an exception.

Fragmented message protocol is very simple:

byte = UdpSendOption.Fragment // 11
ushort = fragments count
ushort = fragmented message id
... // remaining fragmented data to be reconstructed

Then it gets stored on the receiving end and is reconstructed when received fragmentsCount fragments
Currently SendOption.Reliable is assumed for all fragmented messages but maybe it should be specified explictly in case SendOption.ReliableOrdered or something was ever added? e90317b

MTU discovery is somewhat inspired by LiteNetLib, it uses just a few MTU values to discover MTU very quickly and the value is good enough in most cases:

/// <summary>
/// Popular MTU values used for quick MTU discovery
/// </summary>
public static ushort[] PossibleMtu { get; } =
{
576 - MaxUdpHeaderSize, // RFC 1191
1024,
1460 - MaxUdpHeaderSize, // Google Cloud
1492 - MaxUdpHeaderSize, // RFC 1042
1500 - MaxUdpHeaderSize, // RFC 1191
};

It works by sending a reliable packet with the size of the tested MTU, if we get an ACK back it means that the MTU is fine and attempt higher MTU, if not we get an ACPI error back in form of a SocketException and cancel the ACK.

MTU changing (both higher and lower) during connection isn't handled (yet).

This also doesn't take DTLS in account at all, I guess we could subtract DTLS application data header from the MTU when DTLS is used?

buffer[7] = sendOption;
}

Buffer.BlockCopy(data, fragmentDataSize * i - (includingHeader ? 0 : 1), buffer, FragmentHeaderSize + (includingHeader ? 1 : 0), dataLength - (includingHeader ? 1 : 0));
Copy link
Owner

@willardf willardf Dec 17, 2021

Choose a reason for hiding this comment

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

Calculate the headerOffset/Length once so this looks nicer.

var fragmentsCount = messageReader.ReadUInt16();
var fragmentedMessageId = messageReader.ReadUInt16();

lock (_fragmentedMessagesReceived)
Copy link
Owner

Choose a reason for hiding this comment

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

I think you should use the ConcurrentDictionary locking pattern in UdpConnectionListener where you tryget, lock, tryadd. Then once you have a unified FragmentedMessage instance you can lock that instead. Because right here you have a global lock with a decent amount of work in it and I don't think it's necessary.

Keep in mind I have not fully thought this through and I don't personally expect this to be a hot path, so it's a somewhat low pri optimization.

_fragmentedMessagesReceived.Add(fragmentedMessageId, fragmentedMessage = new FragmentedMessage(fragmentsCount));
}

var buffer = new byte[messageReader.Length - messageReader.Position];
Copy link
Owner

Choose a reason for hiding this comment

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

Depending how things go, you might not need to do this copy. We're in control of whether Fragmented packets are recycled or not, so ... they probably aren't? So just capture the whole packet. You can adjust the offset or position if you need, but saving a whole copy is great.

@willardf willardf force-pushed the main branch 8 times, most recently from f30e956 to ff5f86a Compare February 29, 2024 21:01
@NikoCat233
Copy link

This can be useful to resolve the +25 modded protocol huge size packets issue at network interface
Looking forward to this new function

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.

3 participants