Add async way to read early data from TLSAcceptor#73
Add async way to read early data from TLSAcceptor#73tahmid-23 wants to merge 2 commits intorustls:mainfrom
Conversation
cpu
left a comment
cpu
left a comment
There was a problem hiding this comment.
Thanks for the PR. I think you have some CI failures to fixup.
I haven't done a review pass yet, just wanted to leave one comment up-front.
tahmid-23
force-pushed
the
main
branch
2 times, most recently
from
9a37712 to
2565c62
Compare
| } | ||
|
|
||
| if stream.session.is_handshaking() { | ||
| return Poll::Pending; |
There was a problem hiding this comment.
Should we return pending here? or should I handshake? I'm worried about a hang due to missing wake.
There was a problem hiding this comment.
I don't think there would be a missing wake? since handshake is done once the client sends it's finished, and this code depends on read. I'm not fully sure though.
but now that I think about it, the test code doesn't cover this. since I do acceptor.accept(&mut sock).await, the handshake already finishes, so reading the early data "async" effectively did nothing.
should I change some of the server stream logic to use the early data state (kind of like client)? acceptor.accept would no longer wait for the handshake to complete, and writing/reading would need to wait on the handshake to complete.
if I do that, then maybe it would make sense to further the handshake here?
There was a problem hiding this comment.
If we got Pending from stream.read_io() it's fine to return Pending here, can is_handshaking() return true after read_io() has yielded Ok(0)?
| let mut stream = | ||
| Stream::new(&mut this.io, &mut this.session).set_eof(!this.state.readable()); | ||
|
|
||
| match &this.state { |
There was a problem hiding this comment.
Nit: would be nice to restructure this code more like this:
match &this.state {
TlsState::Stream | TlsState::WriteShutdown => {}
TlsState::ReadShutdown | TlsState::FullyShutdown => return Poll::Ready(Ok(())),
s => unreachable!("server TLS can not hit this state: {:?}", s),
}
let mut stream = stream.as_mut_pin();
..| while !stream.eof && stream.session.wants_read() { | ||
| match stream.read_io(cx) { | ||
| Poll::Ready(Ok(0)) => { | ||
| break; | ||
| } | ||
| Poll::Ready(Ok(_)) => (), | ||
| Poll::Pending => { | ||
| break; | ||
| } | ||
| Poll::Ready(Err(err)) => return Poll::Ready(Err(err)), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This code looks it's duplicated from somewhere else. Can we abstract over it instead? If not, why not?
There was a problem hiding this comment.
some of the branches update different variables, I don't really know how to extract it
| data: &[u8], | ||
| vectored: bool, | ||
| ) -> io::Result<(TlsStream<TcpStream>, Vec<u8>)> { | ||
| ) -> io::Result<(client::TlsStream<TcpStream>, Vec<u8>)> { |
There was a problem hiding this comment.
This change seems unrelated? If so, prefer to avoid it.
There was a problem hiding this comment.
I gave a more qualified name because I use server::TlsStream as well. should I still revert it?
| let mut stream_wrapper = TlsStreamEarlyWrapper { inner: stream }; | ||
| stream_wrapper.read_to_end(&mut buf).await.unwrap(); | ||
| let mut stream = stream_wrapper.inner; | ||
| stream.write_all(b"EARLY:").await.unwrap(); |
There was a problem hiding this comment.
Should there be a test that explicitly tests for being able to read actual data off of the early data stream?
There was a problem hiding this comment.
I don't follow. This is already reading the early data?
4 participants
Enables reading early data server-side in an async way, separate from the normal reading method.
Please give any suggestions 🙂