cloudflare / boring

BoringSSL bindings for the Rust programming language.
367 stars 114 forks source link

Remove strict type requirement of `TokioIo` in `v1::HttpsConnector`? #295

Open PaulDance opened 1 week ago

PaulDance commented 1 week ago

Dear maintainers,

I have just recently migrated a non-trivial codebase to the official Hyper v1 support. In doing so, I ran into a peculiar issue: the fact that v1::HttpsConnector changes its response type requirement from only T: AsyncRead + ... to TokioIo<T> where T: AsyncRead + .... This made things fail to compile suddenly.

Apart from the fact that it introduces an inconsistency between the two APIs, it more importantly seems to add awkwardness more that anything? Indeed, the connector I was giving to HttpsConnector::with_connector was a BoxService<Uri, Box<dyn hyper::rt::Read + hyper::rt::Write + ..., Box<dyn Error + ...>> and it worked fine with a cherry-picked version of the v1 support prior to the merge that had T: hyper::rt::Read + hyper::rt::Write + ... as a response type requirement. At this point, the compilation errors were simply about the fact that TokioIo was missing. However, simply adding a .map_response(TokioIo::new) to the connector before giving it to HttpsConnector::with_connector did not suffice: it would fail to prove that the response type Box<dyn ...> was AsyncRead + ..., which would have not been easy to add. Instead, doubling the response mapping, so .map_response(TokioIo::new).map_response(TokioIo::new), is the only thing I could find that restores compilation.

This seems logical considering the new response type requirement that is a fixed TokioIo<T> where T: AsyncRead + ...: wrapping once passes the immediate type checking, but the bounds check still fails as my inner type is still only hyper::rt::Read + ..., while TokioIo<T> becomes AsyncRead + ... due to its trait blanket implementations, thus explaining why TokioIo<TokioIo<T>> is accepted. This necessity to flip-flop seems really weird at first glance.

As the Tower + Hyper + Tokio + etc. stack makes errors long and difficult to understand, it took me a while to get there, so I think something should be done in order to avoid that again in the future. Would it therefore be possible to at least reconsider this fixed type requirement? From my understanding, it should work to keep the simple T: AsyncRead + ... and just document that TokioIo may be used in order to map the response type only once if needed, no? Also, I did not see the v1 module using this TokioIo very much, apart in some cases where TokioIo::new was explicitly called: in the worst case, it can be used as such for HttpsConnector::with_connector as well, right?

Cheers, Paul.