-
Notifications
You must be signed in to change notification settings - Fork 124
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Access application context from tls connection #2448
base: main
Are you sure you want to change the base?
Conversation
Hi WeiChao. Are you still planning to work on this? |
sure, this is initial implementation, Is it right way fix this issue? |
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.
Overall looks good! We'll need to get the s2n-tls method merged and released before merging this though.
quic/s2n-quic-core/src/crypto/tls.rs
Outdated
@@ -130,6 +130,8 @@ pub trait Context<Crypto: crate::crypto::CryptoSuite> { | |||
//# peer's Finished message. | |||
fn on_handshake_complete(&mut self) -> Result<(), crate::transport::Error>; | |||
|
|||
/// Transfer application context from TLS connection to quic connection | |||
fn on_application_context(&mut self, _context: Option<Box<dyn Any + Send + Sync>>) {} |
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.
I think we can leave out the default impl here
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.
done
@@ -155,6 +156,10 @@ impl tls::Session for Session { | |||
context.on_tls_exporter_ready(self)?; | |||
self.handshake_complete = true; | |||
} | |||
// TODO Add new s2n-tls new api, take and put in quic::connection | |||
let ctx: Option<Box<dyn Any + Send + Sync>> = |
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.
Does this need a 'static
as well?
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.
Any already has 'static https://doc.rust-lang.org/std/any/trait.Any.html
Does you add |
try fix #2437