-
Notifications
You must be signed in to change notification settings - Fork 86
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
fix: Fix EOF spin loop by removing intermediate buffer in LazyConfigAcceptor #87
fix: Fix EOF spin loop by removing intermediate buffer in LazyConfigAcceptor #87
Conversation
Thanks for your investigation and for submitting patches! I knew there might be an issue but the original reporter was never able to isolate it enough to make it easy for me to reduce, so that fell by the wayside. Is it correct that with this PR obviates #86? |
So, I was able to confirm in production that this PR seems to fix the CPU issue: This is how it looked before the fix (it immediately spiked):
Correct. |
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.
This looks pretty great, gotta love changes that net remove code while fixing a bug.
Question: how hard would it be to add a test that proves that the spin loop behavior your observed can no longer happen? I think that would be a great addition.
tokio-rustls/src/common/mod.rs
Outdated
@@ -343,5 +326,24 @@ where | |||
} | |||
} | |||
|
|||
/// An adaptor that implements a [`Read`] interface for [`AsyncRead`] types and an | |||
/// associated [`Context`]. | |||
pub struct Reader<'a, 'b, T> { |
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.
Maybe we could give this a more descriptive name now that it's used in other places as well? Maybe something like SyncReadAdapter
? (It appears adapter
is a more common spelling than adaptor
.)
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.
Great idea, done 👍
(Out of curiosity: is netlify using rustls in production? What for?) |
Test added ✅
We're using it in the service that terminates TLS connections at the edge. I. e. all the TLS traffic we have goes through rustls actually. Except for this issue we've had zero trouble with it so far, great work! |
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.
Awesome work, I'll let @quininer take a look before merging this, hopefully we can do a fresh release soon after.
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.
LGTM
Thank you for merging this so quickly! Can you get this out in a point release? |
@NeoLegends 0.23.2 is up on crates.io now. |
This PR removes the intermediate buffer (thereby also fixes #85, I believe) in
LazyConfigAcceptor
, by reusing theAsyncRead
-Read
bridgeReader
struct in the common module. I believe this lets rustls read right from theIO
passed in.Looking forward to your review!