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

Feature request: send proto upgrade Response from handler. #3157

Open
nurmohammed840 opened this issue Mar 3, 2023 · 2 comments
Open

Feature request: send proto upgrade Response from handler. #3157

nurmohammed840 opened this issue Mar 3, 2023 · 2 comments
Labels
C-feature Category: feature. This is adding a new feature.

Comments

@nurmohammed840
Copy link

nurmohammed840 commented Mar 3, 2023

The upgrade example from 1.0.0-rc.3 looks odd (in my opinion).

In web socket context, If the server decides to upgrade the connection, It just send a 101 Switching Protocols response, and immediately use internal TCP as WebSocket protocol.

No need for extra task::spawn:

tokio::task::spawn(async move {
match hyper::upgrade::on(&mut req).await {

Sure, Its only possible, If there is an option to write response directly (rather then returning writable Response object)

Maybe add such API ?

For example:

async handle(req: Request<Incoming>) -> Result<Response<..> {
    match hyper::upgrade::send_response_and_upgrade(resp_body, req).await {
        Err(msg) => eprintln!("upgrade error: {msg}"),
        Ok(upgraded) => { ... }
    }
}
@nurmohammed840 nurmohammed840 added the C-feature Category: feature. This is adding a new feature. label Mar 3, 2023
@nurmohammed840 nurmohammed840 changed the title Feature request: send Response from handler. Feature request: send proto upgrade Response from handler. Mar 3, 2023
@nurmohammed840 nurmohammed840 closed this as not planned Won't fix, can't repro, duplicate, stale Aug 30, 2023
@Noah-Kennedy
Copy link
Contributor

@nurmohammed840 why was this closed?

@A248
Copy link

A248 commented Nov 30, 2023

To elaborate on this issue. The serverside API design fits into hyper's model whereby a Service generates a response from a request; that is, a Service is more or less a Fn(Request) -> Response adorned with generics, futures, error handling.

Before a HTTP upgrade can happen, Hyper has to send the response to client indicating to do so. Thus, the client must receive the response before the server code can re-use the TCP stream. This necessitates that hyper::upgrade::on return a future which completes after the client has processed the request -- a future which necessarily completes after the function return, because if it completed before the function returned, you'd have access to the TCP stream before the upgrade had been negotiated and the HTTP protocol had been concluded.

I do agree, and I came here to write, that the current API deserves to be more flexible. The existing API heavily pushes users to spawn async tasks which entails unnecessary context switching and ownership movement. Working around the order-of-operations problem I described above is possible, and I've written code under the AGPL to do this.

The gains from more ergonomic HTTP upgrades allow callers to keep code organized according to the async model. Spawning a separate task introduces another code branch and thereby pre-empts traditional control flow. In many cases, as is the case here, the extra task is both unnecessary and introduces mental overhead. The framework of async/await was created with simple, sequential-looking, understandable control flow in mind. Hyper should support the sequential, task-free model to make the API and code using it easier to follow.

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

No branches or pull requests

3 participants