feat(s2n-quic-core): add skip functionality to ReceiveBuffer #2050
Conversation
camshaft
force-pushed
the
camshaft/receive-buffer-skip
branch
from
d2db75e to
5152775
Compare
camshaft
force-pushed
the
camshaft/receive-buffer-skip
branch
from
5152775 to
def10e4
Compare
| pub fn skip(&mut self, len: u64) { | ||
| // trim off the data buffer | ||
| unsafe { | ||
| let len = len.min(usize::MAX as u64) as usize; |
There was a problem hiding this comment.
why take a u64 for len instead of usize?
There was a problem hiding this comment.
The caller is using a u64 and would have to convert to usize anyway. The len should never be bigger that 64k so it probably doesn't matter either way. But I can change it to a usize if you think that's clearer.
There was a problem hiding this comment.
I saw the add_start above using usize so I was currious, but as you said it doesn't really matter. If it should never be bigger than 64k should we put a debug_assert! or do something like let len:usize = len.try_into().expect("len must be <= 64")
There was a problem hiding this comment.
I'll try adding an assertion and see if my assumption is true 😁
There was a problem hiding this comment.
turns out it wasn't when writing a FIN segment. I've fixed that and added a test for it as well.
| if len == 0 { | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
Is this an appropriate use of ensure!, or should it just be for error cases?
| if len == 0 { | |
| return Ok(()); | |
| } | |
| ensure!(len > 0, Ok(())); |
There was a problem hiding this comment.
Funny you mention it.. I went back and forth on using it for this. Didn't know if it would be confusing to return Ok or not. What are your thoughts?
There was a problem hiding this comment.
I think its not really confusing (especially with the comment there), but without the return keyword sticking out it takes a couple more cycles to comprehend if you're not used to ensure. But maybe the answer is just to get used to it :-)
2 participants
Description of changes:
The current ReceiveBuffer implementation requires copying all stream data into
BytesMutbuffers to ensure stream data is correctly ordered. This ends up requiring an allocation and a copy, even in scenarios where the stream data being received is at the current offset, and could potentially be copied into an application-provided buffer.This change adds support for that usecase by adding a
skip(len: usize)method that allows a caller to increment the current offset bylenand bypass copying the data into the buffer itself.Testing:
I added a
Skipoperation to the fuzz tests, along with a data checker to ensure the data being written to the buffer is the same that is being read. I also updated the committed corpus to include the new input coverage.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.