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

Erroneous dropped datagrams #2214

Closed
wants to merge 2 commits into from

Conversation

jkalez
Copy link

@jkalez jkalez commented May 16, 2024

Resolved issues:

resolves #2213

Description of changes:

Refactors datagram::Sender::on_transmit to properly handle when datagrams do not fit into a packet. Additionally adds a test for the new implementation, and renames an old test since the name no longer makes sense, given the new implementation.

Call-outs:

None

Testing:

New unit test poll_send_datagram_large

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@camshaft
Copy link
Contributor

What happens when you enqueue a 5000 byte datagram in a 1500 byte MTU?

@jkalez
Copy link
Author

jkalez commented May 17, 2024

Datagrams would block sending until:

  1. the MTU increases such that a Packet can contain the given datagram (unlikely), or
  2. the user removes the datagram from the MTU with retain_datagrams

I had toyed with making the on_transmit implementation smarter, such that it will add whatever datagrams to the packet that it can, even going out-of-order if necessary. For example, if the queue is datagrams of the size [5000, 150, 150, 5000] and an MTU of 1500, datagrams 2 and 3 would eventually be sent, while datagrams 1 and 4 will not until the MTU changes. However I wanted this MR to retain as much of the original behavior of on_transmit as possible, while also correcting the behavior

@camshaft
Copy link
Contributor

Datagrams would block sending until the user removes the datagram from the MTU with retain_datagrams

This is a pretty significant change in behavior. Blocking queues is going to cause all sorts of issues and is why the default implementation is the way it is currently. If the head of the queue isn't able to be transmitted with the current MTU then it's unlikely to ever be transmitted.

If you're wanting different behavior you're free to implement your own datagram provider. That's exactly why this functionality is a provider: the default semantics won't make everyone happy.

@jkalez
Copy link
Author

jkalez commented May 17, 2024

Fair enough, we'll likely implement our own Sender then.

I would argue that dropping the datagram silently without any warning is not what most would expect. I agree that the default semantics are never going to make everyone happy, however a small warning emitted or an Error bubbled up to the caller seems entirely reasonable and something that I think the vast majority of users would appreciate and would not change the current semantic behavior of the on_transmit function.

If you'd like to keep the current behavior as the default, that is understandable. However there's likely still a bug in the waker behavior. When that too-large-to-send datagram is dropped from the head of the queue, the waker is not woken. Therefore, if the queue is full and someone is waiting to add to the queue, they won't be notified that the queue now has empty space. This will happen whenever the queue is full of too-large-to-send datagrams.

If you'd like, I can modify #2213 to better capture a warning/error as a proposed solution, plus a fix to the waker behavior.

Additionally, if the following is true (which I agree it is):

If the head of the queue isn't able to be transmitted with the current MTU then it's unlikely to ever be transmitted.

Then users should not be able to send_datagram, send_datagram_forced, or poll_send_datagram if the size of the datagram is > than the MTU. Those should all return an error. At least then the application would be able to make some informed decision about what it should do with it's too large datagram, versus just hoping it gets sent, when it never will be.

@camshaft
Copy link
Contributor

If you'd like, I can modify #2213 to better capture a warning/error as a proposed solution, plus a fix to the waker behavior.

Yes let's do that. Thanks!

@jkalez
Copy link
Author

jkalez commented May 21, 2024

Done, closing PR.

@jkalez jkalez closed this May 21, 2024
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.

Large datagrams erroneously dropped by default datagram Sender
2 participants