Conversation
9627e91 to
cf4f831
Compare
This covers additional items from cloudflare#2492 that aren't implemented in merged cloudflare#2697. * 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 remove the guard and check for the STREAM frame's acutal header size. * If there's not enough space for stream frame header we incorrectly removed the stream from flushable. This can permantely strand the stream! * Preserve the legacy recovery threshold independently of STREAM header sizing and derive the CRYPTO overhead from its bounded offset. * MAX_CRYPTO_OVERHEAD was incorrect. Fixed it and replaced the hard-coded magic number with the actual calculation.
cf4f831 to
49144fd
Compare
| None => { | ||
| let priority_key = Arc::clone(&stream.priority_key); | ||
| self.streams.remove_flushable(&priority_key); | ||
| Some(v) if v > 0 || stream.send.empty_fin_next() => v, |
There was a problem hiding this comment.
Can we keep the existing Some(v) => v arm and remove empty_fin_next()? When v == 0, emit(&mut []) already returns (0, true) for an empty FIN, while the existing len == 0 && !fin check below handles non-FIN frames. Only None needs the new leave-flushable, rotate, and break behavior. This also removes the new SendBuf method and its dedicated test.
There was a problem hiding this comment.
yes. Good point.
Or should we remove the if len ==0 && !fin block below instead? If we keep the block we need to duplicate the "didn't send anything" code of setting stream_data_skipped and shuffling the streams twice. Once in the _ => block and once in the if-block below.
I think that would be the better choice. Up to you.
| /// 1-byte type + varint-encoded offset (bounded by `MAX_CRYPTO_STREAM_OFFSET`) | ||
| /// + 2-byte length (we always encode as 2 bytes) | ||
| pub const MAX_CRYPTO_OVERHEAD: usize = | ||
| 1 + octets::varint_len(crate::MAX_CRYPTO_STREAM_OFFSET) + 2; |
There was a problem hiding this comment.
MAX_CRYPTO_STREAM_OFFSET only limits received CRYPTO data, while the outgoing stream has a u64::MAX send limit. Could we instead calculate the header size from the actual outgoing crypto_off and proceed only when at least one payload byte fits? Since this is independent of the STREAM fix, could this be done in a separate PR?
2 participants
This covers additional items from #2492 that aren't implemented in merged #2697.
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 remove the guard and check for the STREAM frame's acutal header size.
If there's not enough space for stream frame header we incorrectly removed the stream from flushable. This can permantely strand the stream!
Preserve the legacy recovery threshold independently of STREAM header sizing and derive the CRYPTO overhead from its bounded offset.
MAX_CRYPTO_OVERHEAD was incorrect. Fixed it and replaced the hard-coded magic number with the actual calculation.