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

Large datagrams erroneously dropped by default datagram Sender #2213

Open
jkalez opened this issue May 16, 2024 · 1 comment
Open

Large datagrams erroneously dropped by default datagram Sender #2213

jkalez opened this issue May 16, 2024 · 1 comment
Labels
bug Something isn't working priority/medium Rank 3 size/small

Comments

@jkalez
Copy link

jkalez commented May 16, 2024

Problem:

Datagrams which do not fit within a Packet are erroneously dropped on on_transmit.

It appears the current implementation (quic/s2n-quic-core/src/datagram/default.rs) attempts to add datagrams to a packet as follows:

  1. check if there is pending stream data and if the packet is prioritizing datagrams. If there is pending stream data and the packet is not prioritizing datagrams, return early w/o writing any datagrams to the packet.
  2. While there is some remaining capacity in the packet
    a. pop a datagram off the internal queue
    b. ensure this datagram fits into the existing packet, and write it to the packet if it does. Wake the waker if there is one
    c. if it does not fit, push the packet back into the queue only the procedure has already written at least one datagram

Consider the following example:

  • Queue: A single 1200 byte datagram
  • Packet: remaining capacity of 1000 bytes
  • on_transmit is called
    • The datagram is popped off the queue
    • The datagram does not fit into the packet
    • has_written has never been set to true, so the datagram is never returned to the queue
    • The queue is now empty, and the waker has not been woken

There may be other edge cases not yet considered, but this one most obviously demonstrates the problem.

Solution:

A small refactor of the on_transmit implementation should suffice. Specifically, revisiting the role of has_written as a guard condition before adding popped datagrams back into the queue.

  • Does this change what s2n-quic sends over the wire? --> technically, yes, by sending datagrams which were previously erroneously dropped. Serialization and such of these datagrams do not change, so in that respect, what is sent over the wire does not change.
  • Does this change any public APIs? --> no

Requirements / Acceptance Criteria:

  • RFC links: N/A
  • Related Issues: None that I could find
  • Will the Usage Guide or other documentation need to be updated? No
  • Testing: A new unit test to exercise the above example

Out of scope:

N/A

@jkalez
Copy link
Author

jkalez commented May 21, 2024

After discussion in #2214, it appears pieces of this are intended behavior. However a proper fix is still required for two items:

  1. Users should be notified if their datagrams are dropped due to not fitting in the first available packet, either by a warning message, error, or some other means
  2. The waker behavior in the default Sender is still buggy. If a too-large-to-send datagram is dropped due to its size, the waker is never woken. A proper fix will ensure the waker is woken whenever an item is removed from the internal queue

@camshaft camshaft added bug Something isn't working priority/medium Rank 3 size/small labels Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority/medium Rank 3 size/small
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants