-
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
Add poll_write docs #73
Conversation
(Asked a bunch of questions in #58.) |
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.
Sorry to be negative, but I think this documentation as written here would not help me as a library user, so I hope we can refine it to be more helpful.
tokio-rustls/src/lib.rs
Outdated
//! Most TLS implementations will have an internal buffer to improve throughput, | ||
//! and rustls is no exception. | ||
//! | ||
//! When we write data to `TlsStream`, we always write rustls buffer first, | ||
//! then take out rustls encrypted data packet, and write it to data channel (like TcpStream). | ||
//! When data channel is pending, some data may remain in rustls buffer. | ||
//! at this point we must call flush to write again it into data channel. | ||
//! | ||
//! `tokio-rustls` To keep it simple and correct, [TlsStream] will behave like `BufWriter`. |
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 find this hard to understand. I feel like there are three steps here that should be plainly stated:
- When or in what kind of situations this problem arises ("Does this apply to me")
- Context on the underlying problem; there is some of this, but as written it's hard to relate to as a library user
- What to do to solve the issue (that is, in what kind of situations should I be calling
poll_flush()
explicitly)
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.
Sorry, I think it is common sense that BufWriter
needs to call flush
. so I subconsciously did not give more explanation.
//! We did this in the early days of `tokio-rustls`, but it caused some bugs. | ||
//! We can solve these bugs through some solutions, but this will cause performance degradation (reverse false wakeup). | ||
//! | ||
//! And reverse write will also prevent us implement full duplex in the future. | ||
//! | ||
//! see <https://github.com/tokio-rs/tls/issues/40> |
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 don't understand who the intended audience for this section is; that is, which (kind of) library users care about this problem?
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.
People often ask me why tokio-rustls
needs to call flush
but tokio-native-tls
does not. why does tokio-rustls
not write data during poll_read
. these are real user questions.
tokio-rustls/src/lib.rs
Outdated
//! native-tls also needs to call flush, but it will be much less. | ||
//! | ||
//! When data channel returns to pending, `native-tls` will falsely report the number of bytes it consumes, | ||
//! which does not conform to convention of `AsyncWrite` trait. | ||
//! This means that if you give inconsistent data in two `poll_write`, it may cause unexpected behavior. |
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, too, it's not super clear to me who would ask this kind of question or why it's relevant.
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 still don't like this text much, but on the other hand I don't have energy right now to rewrite it into something better and I don't want to block the release any more, so let's just merge it, we can always improve them later.
Close #58