conn: support BufRead on Reader

[horazont]

This exposes the internal buffer, which allows for zero-copy use of the received plaintext data.

This has a caveat though: The Reader object is borrowed and created from the ConnectionCommon, which means that it is not possible to use this in all situations. For instance, a downstream implementation of BufRead which uses a ConnectionCommon (or a Deref<Target = ConnectionCommon>), would in fact not be possible: it would have to call .reader() on the connection, which would then return a temporary and from that temporary, it is impossible to return the result of fill_buf().

To fix this, BufRead would have to be exposed on ConnectionCommon.

---

I opened this as a PR, because I think working code is always a nice way to start talking. I expect that you have comments on this, one way or the other.

I would suggest to implement BufRead, Read and Write directly on ConnectionCommon instead of this. That would be the only way to actually make use of BufRead in many situations due to the constraints outlined above. However, I assumed that you have reasons for not exposing the traits on ConnectionCommon right away, hence this PR to get the discussion started.

I do have local uncommitted code which implements BufRead on ConnectionCommon, as well local uncommitted code which, based on that, implements AsyncBufRead on tokio_rustls::TlsStream.

[codecov-commenter]

Codecov Report

Merging #870 (75fd6c1) into main (c253d68) will decrease coverage by 0.13%. The diff coverage is 38.09%.

:exclamation: Current head 75fd6c1 differs from pull request most recent head cf8e797. Consider uploading reports for the commit cf8e797 to get more accurate results

@@            Coverage Diff             @@
##             main     #870      +/-   ##
==========================================
- Coverage   96.23%   96.10%   -0.14%     
==========================================
  Files          59       59              
  Lines        9441     9457      +16     
==========================================
+ Hits         9086     9089       +3     
- Misses        355      368      +13     

Impacted Files	Coverage Δ
rustls/src/vecbuf.rs	94.20% <25.00%> (-4.29%)	:arrow_down:
rustls/src/conn.rs	94.50% <41.17%> (-1.81%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c253d68...cf8e797. Read the comment docs.

[djc]

You can read up on context in https://github.com/rustls/rustls/issues/563. My recollection we previously implemented some of those traits for ClientSession and ServerSession types but stopped because we wanted to make backwards-incompatible changes with how those impls worked; we introduced the accessor type to effectively break compatibility in an obvious way. It's only later that we exposed the ConnectionCommon type publicly, but since IIRC ClientConnection/ServerConnection still (indirectly) Deref to ConnectionCommon that would have been a reason not to implement those types for ConnectionCommon directly.

For the future, I agree that it might make sense to expose some I/O traits on ConnectionCommon directly.

[horazont]

Aha thank you for the context. That makes a lot of sense. Using WouldBlock as the current implementation does also make a lot of sense to me.

I would be happy to redo this patch to implement Read, Write and BufRead on ConnectionCommon, if that is more sensible?

[djc]

Probably good to wait for feedback from @ctz before you do work that ends up being undesirable.

[horazont]

Affirmative!

[horazont]

Uh :-).

Some feedback would've been nice :).

I realize that this's been open for nearly two years and quite some things have changed in rustls. AFAICT from a quick glance at the documentation, the ClientConnection reader still doesn't offer BufRead though.

So for future readers, it'd be good to have a comment here which states whether this was closed out of principle (BufRead is for some reason not suitable/achievable in the current design) or out of deprecation of the approach (the PR is, after all, nearly two years old).

[ctz]

Apologies, this was more a time-out than a "never". But soon we plan to add lower-level APIs that don't keep an internal per-connection buffer, which should give a more powerful and composable route to getting to a BufRead without duplicating buffer contents.