-
Notifications
You must be signed in to change notification settings - Fork 619
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
move sendable_plaintext from CommonState to ConnectionCommon #1591
Conversation
72a497b
to
a9ce3ae
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1591 +/- ##
==========================================
- Coverage 95.60% 95.59% -0.01%
==========================================
Files 79 79
Lines 16662 16704 +42
==========================================
+ Hits 15930 15969 +39
- Misses 732 735 +3 ☔ View full report in Codecov by Sentry. |
a9ce3ae
to
5c849ac
Compare
@cpu only the last 3 commits. I have rebased the PR |
5c849ac
to
8602a5e
Compare
For stacked PRs, I recommend setting the PR's base commit as the branch it's stacked on. This makes it much more obvious what's going on (and the PR's will be reset to main as soon as the base is merged). |
@djc that's what I normally do when I can push branches into the repository where I'm submitting the PRs to but these PRs come from branches on my fork so I can't set the base to a branch in my fork and have the PR be visible on rustls/rustls; GitHub will open the PR in japaric/rustls where you won't see it / get notified |
8602a5e
to
a888023
Compare
I rebased this. |
rustls/src/client/tls12.rs
Outdated
@@ -1032,7 +1032,8 @@ impl State<ClientConnectionData> for ExpectFinished { | |||
emit_finished(&st.secrets, &mut st.transcript, cx.common); | |||
} | |||
|
|||
cx.common.start_traffic(); | |||
cx.common | |||
.start_traffic(cx.sendable_plaintext.as_deref_mut()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From looking at the diffs, it doesn't seem like we need as_deref_mut()
here?
@@ -348,24 +356,29 @@ impl CommonState { | |||
self.send_appdata_encrypt(data, limit) | |||
} | |||
|
|||
pub(crate) fn start_outgoing_traffic(&mut self) { | |||
pub(crate) fn start_outgoing_traffic( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, and for start_straffic()
, it would be good to add a comment about when/why sendable_plaintext
is None
.
@@ -715,6 +728,7 @@ pub(crate) trait State<Data>: Send + Sync { | |||
pub(crate) struct Context<'a, Data> { | |||
pub(crate) common: &'a mut CommonState, | |||
pub(crate) data: &'a mut Data, | |||
pub(crate) sendable_plaintext: Option<&'a mut ChunkVecBuffer>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to add comments here too about the Some
vs None
cases.
This generally makes sense to me. |
a888023
to
3c24390
Compare
add a `set_buffer_limit` method to `Connection` to minimize breakage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍
Benchmark resultsInstruction countsSignificant differencesThere are no significant instruction count differences Other differencesClick to expand
Wall-timeSignificant differencesThere are no significant wall-time differences Other differencesClick to expand
Additional informationCheckout details:
|
this is a refactor that follows up from #1583 and removes one of the 3 internal buffers in
CommonState
so that theUnbufferedConnection
API does not inherit it.breaking changes:
CommonState::set_buffer_limit
was moved intoConnectionCommon
. this is technically a breaking change but users of theset_buffer_limit
API throughConnectionCommon
andConnection
won't notice the change. in fact, none of the existing examples / tests required changes due to this "breaking" changethis PR is best reviewed on a commit by commit basis (do skip the PR1583 commits)