Skip to content
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

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

Closed
PaulDance opened this issue Nov 22, 2024 · 0 comments · Fixed by #305
Closed

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

PaulDance opened this issue Nov 22, 2024 · 0 comments · Fixed by #305

Comments

@PaulDance
Copy link
Contributor

PaulDance commented Nov 22, 2024

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?

I've made an attempt at this in #305.

Cheers,
Paul.

PaulDance added a commit to JustRustThings/boring that referenced this issue Dec 6, 2024
…oring::v1::HttpsConnector`

Closes cloudflare#295.

Signed-off-by: Paul Mabileau <[email protected]>
PaulDance added a commit to JustRustThings/boring that referenced this issue Dec 7, 2024
…oring::v1::HttpsConnector`

Closes cloudflare#295.

Signed-off-by: Paul Mabileau <[email protected]>
kornelski pushed a commit that referenced this issue Dec 7, 2024
…oring::v1::HttpsConnector`

Closes #295.

Signed-off-by: Paul Mabileau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant