-
Notifications
You must be signed in to change notification settings - Fork 25
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
Issue 245: Async token refresh #251
Conversation
Signed-off-by: Wenqi Mou <[email protected]>
Signed-off-by: Wenqi Mou <[email protected]>
Signed-off-by: Wenqi Mou <[email protected]>
Signed-off-by: Wenqi Mou <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #251 +/- ##
==========================================
- Coverage 78.47% 78.38% -0.09%
==========================================
Files 51 51
Lines 10412 10541 +129
==========================================
+ Hits 8171 8263 +92
- Misses 2241 2278 +37
|
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.
Apart from the code duplication comment the changes look good.
Signed-off-by: Wenqi Mou <[email protected]>
Signed-off-by: Wenqi Mou <[email protected]>
Signed-off-by: Wenqi Mou <[email protected]>
Signed-off-by: Wenqi Mou <[email protected]>
self.stream | ||
.as_mut() | ||
.expect("get connection") | ||
.flush() |
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 API Flushes this stream, ensuring that all intermediately buffered contents reach their destination.
https://docs.rs/tokio-rustls/0.22.0/src/tokio_rustls/client.rs.html#93-94 snippet indicates a TlsStream.flush()
is required to ensure data actually reaches the destination.
Is this different with a TcpStream ?
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.
yeah I think TcpStream itself doesn't have a buffer and any write is directly sent over the wire. However this TlsStream has a buffer inside that needs to be flushed. The name TlsStream
is kind of misleading.
Change log description
Currently token refresh is using a Runtime which makes it impossible to work in a async context. Use async way to refresh the token instead.
Purpose of the change
Fix #245
What the code does
Use async way to refresh token