fix: do not send zero-length non-FIN stream frames

[gregor-cf]

While these frames are valid per RFC 9000, it is pointless to send them (and they triggered another bug on the receive side).

The MAX_STREAM_OVERHEAD constant we used to guard the path that writes STREAM frames was too low; STREAM frame headers can be up to 19 bytes. However, using the maximum overhead is a bit iffy as well, since it would prevent us from writing the more common, smaller frames. So instead, we guard on MIN_STREAM_OVERHEAD and add a secondary check to ensure the remaining capacity is enough to fit at least one byte of stream payload.

While doing this, I also noticed that we were removing the stream from the flushable queue when there was not enough space for its header. That could strand the stream permanently and may also account for connections running out of flow-control send credit. Fixed that as well.

The MAX_STREAM_OVERHEAD constant was also used in the recovery on_app_limited() check, so I had to tweak that logic too.

[antoniovicente]

Technically the +1 is only needed in cases where the stream has at least 1 byte to send. The case of stream FIN with no data could be transmitted with v == 0.

[LPardue]

+1 I think we need a little more nuance here

[antoniovicente]

I would start with:

let max_len = match left.checked_sub(hdr_len) {
  Some(v) if v > 0 => v,

The case where emit could return bytes=0 and fin=true is an edge case where I think we could still require v>1.

We could add a method to query if emit will generate a 0 length frame with FIN set and tweak the expression above to:

Some(v) if v > 0 || zero_bytes_with_fin => v,

[antoniovicente]

An implication of break instead of continue is that other streams with lower stream id or offset could have written a tiny frame but now they won't.

I need to look into the implications of the remove_flushable

The check for MAX_STREAM_OVERHEAD in the original code made this branch unlikely to be reached and we could run into problems when we do hit it.

I think with the lower MIN this branch will be easier to test correct. This is a very good thing.

[gregor-cf]

FYI, remove_flushable() means we don't try to emit from this stream again until it's re-inserted into flushable. That only happens when the app writes more data to the stream. If the app already wrote a FIN, the stream will be stranded forever.

I was actually wondering if should remove the MIN_STREAM_OVERHEAD guard altogether and just rely on this check here. As soon as a stream has offset>64 we're past the guard anyways, so I don't think it really does much and just adds clutter / complexity.

[antoniovicente]

I have no objections to removal of the MIN_STREAM_OVERHEAD guard.

[gregor-cf]

If we want to continue instead of break, we'd just need to add a different iterator for flushable. I think many of the other streams.<adjective> collection already to.
But with the old MAX_STREAM_OVERHEAD guard, we also skipped many streams that would have fit.

[antoniovicente]

I'm fine with the break.

[LPardue]

I think this change in behaviour needs some more consideration, the break risks a high-priority stream blocking forward progress on others and introducing tail latency. Worst case, a certain combo could completely stall out all other streams.

I think I'd be more comfortable with having a min guard (to prevent looping through the whole set when we really tand no hope of sending anything) and trying to walk the entire flushable queue. We could avoid needing a new fluhable iterator type if we store the removed flushable values and reinsert them after we're done sending. But if its easy enough to add a new iterator that would be ok too.

[gregor-cf]

I think this is still a net improvement. The vast majority of cases / streams would have never hit this continue at all (only streams with hdr_len > 12 would have, so essentially only a stream_off of 1GB+ would have). The MAX_STREAM_OVERHEAD guard would have skipped even attempting to fit a frame. Now we at least try to see how much space the header would actually need. In many cases (hdr_len < 12), we can actually send a frame now where we previously couldn't. The break here we mimics the behavior of the old MAX_STREAM_OVERHEAD: if we can't fit the frame, we simply skip the attempt of sending a STREAM until cwnd_available increases again.

The impact on lower priority streams should be minimal, I think. In the best case, another stream could have fit
a max 14bytes of payload into a frame before it also has to wait for more available cwnd.

That said, adding the iterator is pretty straightforward and it's certainly a further improvement.

[LPardue]

yes the new approach does benefit from supporting smaller stream frames.

I think the old bug had an upside. If you have a huge STREAM frame to send and left was between 12 and 20 bytes so you couldn't actually send, the stream would be dropped and since checked_sub didn't update left, the continue an then emit one frame in the same round, and never be worried about the huge frame again 👿

Adding an iterator lets us have the best of both worlds

[gregor-cf]

That said, adding the iterator is pretty straightforward and it's certainly a further improvement.

It turns out, it isn't as trivial, we'd need to create a Vec/copy of all flushable stream_ids. (That's how readable and writable are handled). That seems wasteful since in the general case we only look at the first id. Cases in which it helps are rare (a large stream at the front, but still have additional smaller streams that are flushable), and the benefit it marginal (can send at most 14 more bytes, 1% of a full-sized frame). Plus it adds code complexity.

So TL;DR: I'd leave it as-is.

[antoniovicente]

I'm fine with leaving this as-is.

[antoniovicente]

I think we discovered that a more correct number is 20. But I'm fine keeping it as it for consistency.

[gregor-cf]

:-)

[gregor-cf]

addressed all the comments