Repository navigation
Conversation
Make sure to requeue a datagram and let the caller know that it could not be sent. An example of this is when a pending ack has consumed space before the datagram.
| connection.log.datapath( | ||
| "datagram for flow \(firstFlowID.debugDescription) does not fit \(roomBeforeWriting) free bytes; requeueing" | ||
| ) | ||
| datagrams.prepend(frame: datagramFrame) |
There was a problem hiding this comment.
Okay, so here we just add the frame back in and frame array and that way pending items knows to build a packet here for that flow again, correct?
There was a problem hiding this comment.
Yes, redo it. If it was a pending ack it would have gone out and then when we return we can pick up an empty packet and then send the datagram.
There was a problem hiding this comment.
In general, this seems like a good idea to not drop locally -- but does this cause a case where not being able to send a particular datagram to be at the front of the list forever and if it is too big to fit, no other datagrams can send?
There was a problem hiding this comment.
Head of line blocking is now solved by dropping anything that will never fit (and an error message).
| connection.log.error("Unable to write datagram for flow \(firstFlowID)") | ||
| // Requeue rather than finalize and pass up the error so a retry is possible | ||
| connection.log.datapath( | ||
| "datagram for flow \(firstFlowID.debugDescription) does not fit \(roomBeforeWriting) free bytes; requeueing" |
There was a problem hiding this comment.
| "datagram for flow \(firstFlowID.debugDescription) does not fit \(roomBeforeWriting) free bytes; requeueing" | |
| "Datagram for flow \(firstFlowID.debugDescription) does not fit \(roomBeforeWriting) free bytes; requeueing" |
A datagram too big for the flow should be dropped and prevent any head-of-line blocking. That works, but add a test to ensure that as well.
| ) | ||
| } catch { | ||
| connection.log.error("Unable to write datagram for flow \(firstFlowID)") | ||
| // Requeue rather than finalize and pass up the error so a retry is possible |
There was a problem hiding this comment.
The only reason why this fails is because we couldn't write, but since we already requested a big enough frame how can this fail?
There was a problem hiding this comment.
Check my math here, assume we used QUIC options to set the initial packet size to 1350. When an upper asks for datagrams what is returned is 1305:
usableDatagramSize = 1350 - 16 tag - 1 base - 20 dcid - 4 pnReserve - 4 dgramFrameHdr = 1305
So the 1305 is the maximum data that could go into a datagram frame if it was the only thing in the packet. But if there is another frame present (here a higher-priority 5 byte ACK frame), there is not room for both the ACK and datagram.
ackFrameLength = 1 type + 1 vle(largestAcked) + 1 vle(ackDelay) + 1 vle(rangeCount) + 1 vle(firstRange) = 5
So the builder needs to realize this, let the ACK go, and then send the datagram. We can go about it in a couple of different ways, I may improve that logic a bit. But I think the math explains why we need some extra logic here unless I'm missing something.
|
Does this fix the same thing as #206? This looks similar to the second commit I had there. In that case, I'll close my PR. Sorry for missing this one. |
Yes, same issue - sorry I missed the overlap as well even though @agnosticdev highlighted it. I started heading towards a different solution that looked a little like yours. I think we need to drop things that will never fit and error. I'll make sure to pick up your unit test for sure. |
that doesn't fit the remaining packet stays queued. Add batch datagram test - both from apple#206
holleman
changed the title
5 participants
Make sure to requeue a datagram and let the caller know that it could not be sent. An example of this is when a pending ack has consumed space before the datagram.