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

Support body write interruption #3121

Open
sfackler opened this issue Jan 18, 2023 · 10 comments · May be fixed by #3169
Open

Support body write interruption #3121

sfackler opened this issue Jan 18, 2023 · 10 comments · May be fixed by #3169
Labels
B-rfc Blocked: More comments would be useful in determine next steps. C-feature Category: feature. This is adding a new feature.

Comments

@sfackler
Copy link
Contributor

If a peer stops writing the body of a request or response, a Hyper client or server has the ability to detect that and handle it however it likes. However, the same is not true if the peer stops reading the body of the request or response that Hyper is sending to it - the write will just indefinitely hang waiting on space in the stream.

There should be some way for consumers to tell Hyper to abort the write. For example, there could be a poll_abort method on Body which Hyper will poll even when it can't request another body chunk. The body implementation could maintain whatever timeout logic it wants internally and ensure the task is woken up at the appropriate time.

@sfackler sfackler added the C-feature Category: feature. This is adding a new feature. label Jan 18, 2023
@seanmonstar
Copy link
Member

I think I get what you mean. That if the IO transport currently doesn't want any more bytes written (or the HTTP/2 stream), then hyper won't be call poll_frame(), and it won't until the the IO says it wants more. But what if you, the user, wanted a timeout to be polled during that time. Currently, it can't be.

Does that sound right?

@seanmonstar seanmonstar added the B-rfc Blocked: More comments would be useful in determine next steps. label Jan 18, 2023
@sfackler
Copy link
Contributor Author

Yep!

@seanmonstar
Copy link
Member

Seems like a good thing to solve. Would you like to propose a solution and/or alternatives? Or anyone else is free to as well.

@sfackler
Copy link
Contributor Author

sfackler commented Jan 24, 2023

The option I mentioned above could look something like

pub trait Body {
    // existing API

    /// Determine if the body is still in a healthy state without polling for the next frame.
    ///
    /// `Body` consumers can use this method to check if the body has entered an error state even when the consumer is not
    /// yet ready to try to read the next frame from the body. Since healthiness is not an operation that completes, this
    /// method returns just a `Result<...>` rather than a `Poll<...>`.
    ///
    /// For example, a `Body` implementation could maintain a timer counting down between `poll_frame` calls and report an
    /// error from `poll_healthy` when time expires.
    ///
    /// The default implementation simply returns `Ok(())`.
    fn poll_healthy(
        self: Pin<&mut Self>,
        cx: &mut Context<'_>,
    ) -> Result<(), Self::Error> {
        Ok(())
    }
}

The nice thing about this is that it enables Hyper to provide the same information with the Body implementation it provides from client responses and server requests. For example, it could alert the consumer to socket disconnects or something like that.

A more limited option could be to allow a frame timeout to be configured but that's inherently going to limit how much control user code has over how it wants to manage this kind of cancellation.

@seanmonstar
Copy link
Member

Ping @davidpdrsn @LucioFranco, anyone else who should see this issue?

@sfackler
Copy link
Contributor Author

I put together an implementation of this.

@LucioFranco
Copy link
Member

I think this is a really interesting and good idea, though the API design having it be a poll fn that doesn't return Poll seems really awkward.

@sfackler
Copy link
Contributor Author

Yeah, should we rename it to is_healthy? We definitely need pass Context in though so you can poll timers or whatever.

@LucioFranco
Copy link
Member

Body consumers can use this method to check if the body has entered an error state even when the consumer is not yet ready to try to read the next frame from the body.

So if I understand correctly, the idea is that generally timeout works fine if the consumer of the body (user of hyper) is polling it but that timer will never get triggered if they don't poll the body. So this is a way to allow hyper to figure out if the body is good or not without initiation a frame read from socket? So this api will never be used by consumers but basically only by hypers connection logic etc?

Yeah, should we rename it to is_healthy? We definitely need pass Context in though so you can poll timers or whatever.

Is there any precedent on fn that take context but are not named poll? I think is_ makes sense, though I am not convinced on "healthy" being the right word.

Also thanks for that example in hyperium/http-body#90 (comment) it was very helpful.

@sfackler
Copy link
Contributor Author

Generally it'd only be used by Hyper. In the future we could implement it for Hyper's own body type so consumers could do the same kind of error short-circuiting, but that's not really the use case I'm interested in.

I'm not aware off the top of my head any non-poll methods taking the context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-rfc Blocked: More comments would be useful in determine next steps. C-feature Category: feature. This is adding a new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants