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

Upgraded::downcast_ref is unusable with hyper_util::server::conn::auto #3704

Open
nox opened this issue Jul 16, 2024 · 4 comments · May be fixed by #3705
Open

Upgraded::downcast_ref is unusable with hyper_util::server::conn::auto #3704

nox opened this issue Jul 16, 2024 · 4 comments · May be fixed by #3705
Labels
C-feature Category: feature. This is adding a new feature.

Comments

@nox
Copy link
Contributor

nox commented Jul 16, 2024

hyper_util::server::conn::auto::Connection<'a, I, S, E> internally wraps I in hyper_util::common::rewind::Rewind<_>, which is a private type, so we cannot ever make use of Upgraded::downcast_ref anymore.

While I could expose Rewind<_> in hyper_util and improve docs in hyper_util::server::conn::auto to signal that one should be downcasting to Rewind<I>, I am filing this in hyper because I thought of a potentially better resolution:

  1. Introduce hyper::server::conn::http1::Builder::serve_buffered_connection:

    impl Builder {
        pub fn serve_buffered_connection<I, S>(&self, buffered: Bytes, io: I, service: S) -> Connection<I, S>
        where
            S: HttpService<IncomingBody>,
            S::Error: Into<Box<dyn StdError + Send + Sync>>,
            S::ResBody: 'static,
            <S::ResBody as Body>::Error: Into<Box<dyn StdError + Send + Sync>>,
            I: Read + Write + Unpin,
        {
            let mut conn = proto::Conn::new_buffered(buffered, io);}
    }
  2. Use that in hyper_util::server::conn::auto::Builder::serve_connection instead of wrapping I into Rewind<_>.
@nox nox added C-bug Category: bug. Something is wrong. This is bad! C-feature Category: feature. This is adding a new feature. and removed C-bug Category: bug. Something is wrong. This is bad! labels Jul 16, 2024
@nox
Copy link
Contributor Author

nox commented Jul 16, 2024

hyper_util::server::conn::auto::Builder itself could also be equipped with new methods serve_buffered_connection and serve_buffered_connection_with_upgrades to accomodate for users doing their own kind of prebuffering (we do such things at work so we also have a type similar to Rewind<_> laying around).

@nox
Copy link
Contributor Author

nox commented Jul 16, 2024

Of course, another solution would be to just move hyper_util::server::conn::auto to hyper::server::conn::auto.

@seanmonstar
Copy link
Member

I want a solution to the problem, definitely.

  • I hope that auto can eventually be moved into hyper from hyper-util, but I do think it needs bake a little more (and know how it can support h3).
  • At least from my feelings, I don't love including a method like serve_buffered_connection. It feels like exposing an internal detail that wouldn't otherwise be something we would expose.

I'm not sure exactly how to allow it, though. I kind of find myself wishing for std::any::provide(), which I know got its scope shrunk to only work with Error now. Is there any other mechanism we haven't thought of?

@nox
Copy link
Contributor Author

nox commented Jul 23, 2024

At least from my feelings, I don't love including a method like serve_buffered_connection. It feels like exposing an internal detail that wouldn't otherwise be something we would expose.

Tbh "I have a buffer of bytes I read and I need to do something like Rewind<_> to feed it back to Hyper" is a thing that comes up a gazillion times here at work, and it always bothers me a little bit because I know Hyper has its own read buffer internally.

Is there any other mechanism we haven't thought of?

Given that auto is supposed to be put back into hyper at some point, maybe we could just expose Rewind and explain that users need to downgrade to Rewind<S> instead of S? Would that be acceptable as a temporary crutch? It's not really usable for us because of this issue right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature Category: feature. This is adding a new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants