hyper icon indicating copy to clipboard operation
hyper copied to clipboard

Support body write interruption

Open sfackler opened this issue 2 years ago • 10 comments

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 avatar Jan 18 '23 20:01 sfackler

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 avatar Jan 18 '23 21:01 seanmonstar

Yep!

sfackler avatar Jan 18 '23 23:01 sfackler

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.

seanmonstar avatar Jan 24 '23 00:01 seanmonstar

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.

sfackler avatar Jan 24 '23 01:01 sfackler

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

seanmonstar avatar Mar 03 '23 14:03 seanmonstar

I put together an implementation of this.

sfackler avatar Mar 11 '23 02:03 sfackler

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.

LucioFranco avatar Mar 14 '23 15:03 LucioFranco

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

sfackler avatar Mar 14 '23 15:03 sfackler

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 https://github.com/hyperium/http-body/pull/90#issuecomment-1468686214 it was very helpful.

LucioFranco avatar Mar 20 '23 15:03 LucioFranco

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.

sfackler avatar Mar 20 '23 16:03 sfackler