Rewrite message deframer #2049
Rewrite message deframer #2049
Conversation
Benchmark resultsInstruction countsSignificant differencesClick to expand
Other differencesClick to expand
Wall-timeSignificant differencesThere are no significant wall-time differences Other differencesClick to expand
Additional informationCheckout details:
|
957f46a to
984a459
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2049 +/- ##
==========================================
+ Coverage 94.29% 94.35% +0.05%
==========================================
Files 97 99 +2
Lines 21913 21879 -34
==========================================
- Hits 20663 20644 -19
+ Misses 1250 1235 -15 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
This is a really nice direction. I know from trying to untangle parts of this code before that it's very non-trivial to navigate the complexity of the task and the requirements imposed by the borrow checker. Kudos for managing 🙇 It was a really interesting exercise to read through this.
Here's an initial pass with some feedback. IMO this feels like a case where individual commits help for review, but it might be better to land as a one-and-done squashed commit. Either that or once it's further along it might be sensible to revisit the intermediate commits to make sure they build/test/lint cleanly.
c4e3b0b to
9d05d70
Compare
|
I think this is ready to go from my side. |
There was a problem hiding this comment.
Here's some initial feedback. So far I have not done an in-depth review yet -- after starting to look at types called Locator and Delocator I was a bit scared so I first want to ask some questions:
- What is the conceptual problem that this refactoring is solving? Presumably you're now close enough that you can articulate it in writing. Would be good to have some high-level design comments on what the important constraints here are and why this is the/an optional solution.
- It looks like this is a rewrite from scratch, and it shows in the commit history, which makes it harder to review.
I guess as a main contributor of the previous deframer (though before it got further complexified in order to support the unbuffered case) I'm still trying to grasp the (design/concept) issues here and how this makes rustls (and our lives as maintainers) better.
Also, the commit history clearly shows this started as a from-scratch rewrite. It would be a lot easier to review this against the existing implementation if the commit history was closer to replacing it in the smallest pieces possible (though I understand this might be hard/labor-intensive).
rustls/src/msgs/deframer/outer.rs
Outdated
| } | ||
| } | ||
|
|
||
| impl<'a, 'b> Iterator for DeframerIter<'a, 'b> { |
There was a problem hiding this comment.
I guess this is similar as the point that @cpu made before, but (perhaps more concretely) this would be easier to review if it was introduced in the same commit that removed the original version of this.
There was a problem hiding this comment.
How about merging together the commit that deletes the old deframer code, the commit that introduces the iterator-based one, and the commit that moves calling code between them. That will be a big commit, but it might be an improvement on the current state?
I would suggest keeping the new handshake deframer introduction in its own commit (before the big one) because it functionally is quite dissimilar to the old code.
d372cba to
226f582
Compare
I think for me the previous deframer was something I could hold my nose at while it worked, but then inscrutable once I had to debug an issue in it. I think the root of that was one piece of code with too many responsibilities. It was a reasonable process how it got there -- having it decrypt the messages and join together the handshake stream was done to achieve very real performance improvements, but it became no longer necessary once the inbound message types started borrowing their data. But, to be more concrete about the actual issue that got me to giving up on the existing deframer: basically #[test]
fn take_panics() {
let mut buf = [1, 2, 3];
let mut dsb = DeframerSliceBuffer::new(&mut buf);
let rs = RawSlice::from(&dsb.filled()[..1]);
assert_eq!(&[1], dsb.take(rs));
// any of the following lines panic (not just because the assert fails)
assert_eq!(&[2, 3], dsb.filled());
assert_eq!(2, dsb.len());
assert!(!dsb.is_empty());
}To reframe that in terms of a conceptual problem, I'd say it is symptomatic of not being precise about what code requires mutable access to the input buffer (and the complexity which came with achieving that). The main reduction in complexity in this PR is achieved in minimising and containing those areas. The other structural issue I didn't like with the existing code was how much complexity budget was applied to having the deframer be involved with writing messages into the underlying buffer for received quic handshake messages. I think the situation at the end of this PR in that area is an improvement. Having spent quite a bit of time on this area of the code so far, I also have some future improvements in mind:
|
Have thought about it a bit more, and I'm going to do this in this PR. |
a5c319d to
72ba041
Compare
Test data extracted from run of unbuffered-client against icanhazip.com
Put it inside the crate and build it unconditionally so breakage is visible without needing to run `cargo fuzz build`. Eliminate the exports under `internals` that exist solely for it.
72ba041 to
2ab347e
Compare
There was a problem hiding this comment.
Sorry for the drive-by review!
I was reading this PR out of curiosity, and found it remarkably readable, but I got nerd-sniped and had a couple of shower thoughts on how it could potentially be improved.
Please do disregard this if the comments aren't helpful!
| // if this is the last handshake message, then we'll end | ||
| // up with an empty `spans` and can discard the remainder | ||
| // of the input buffer. | ||
| let discard = if self.deframer.spans.len() - 1 == self.index { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
It is actually pretty crucial that no "discard" operation happens while there is any partial or full message, as it moves the buffer and therefore invalidates our ideas about where the message fragment is.
| // `_last_incomplete`, and reparse the result. | ||
| // | ||
| // we cannot merge these processes, because `coalesce` mutates the underlying | ||
| // buffer, and `msg` borrows it. |
There was a problem hiding this comment.
After reading this, I wonder if it would make sense for input_message to take msg: InboundUnborrowedMessage and containing_buffer: &mut [u8] instead.
This would allow hiding the coalesce logic inside the handshake deframer instead of exposing it to the callers.
It would require the caller to use .unborrow(), which is ugly, but I think the coalesce() calls are also ugly - it's a case of "pick your poison".
Doing so would simplify the internal invariants - for example:
requires_coalescewould only need to look atself.spans.last()as all previous spans would be guaranteed to be coalesced.
I understand that this split between input_message and coalesce is one of the major design points of this PR, however, so please feel free to ignore this comment.
There was a problem hiding this comment.
I view InboundUnborrowedMessage as a bit of a hack as part of convincing the borrow checker to determine the correct lifetime relationship between the buffer and object returned by deframe(). So while this would work equally well, perhaps better, I'm minded to keep it as is.
2ab347e to
8aa2b71
Compare
`MessageDeframer` is replaced with a combination of the new `deframer::DeframerIter`, `deframer::handshake::HandshakeDeframer` and the existing `RecordLayer`. QUIC code directly inputs data to `HandshakeDeframer`. There is no QUIC-specific code now in either deframer. Some policy code has been hoisted up into `ConnectionCore`. The old `MessageDeframer` is eliminated, as is code which became unused after that.
8aa2b71 to
af08336
Compare
| /// | | ||
| /// spans = [ { bounds = (5, 18), size = Some(9), .. } ] | ||
| /// ``` | ||
| pub(crate) fn coalesce(&mut self, containing_buffer: &mut [u8]) { |
There was a problem hiding this comment.
The addition of the comment with the diagram of buffer contents is really nice, exactly what I was hoping to see. Thank you!
|
Going to merge this with one review, but as always happy to address later comments in a follow-up PR. |
Seems like the captured data is expired |
|
Thanks for the ping -- will address this shortly. |
This PR starts with a repro case for #2040 , with the end goal of a) that and all other tests passing, and b) the deframer code being something I feel I can live with.