Add RequestBodyTimeout and ResponseBodyTimeout
#109
Conversation
| enum State { | ||
| NotPolled(Duration), | ||
| SleepPending(#[pin] Sleep), | ||
| } |
There was a problem hiding this comment.
Not sure how useful this would be in practice. I'm just wondering if users might receive a request and not poll the body immediately and hit the timeout because they were slow to call the first poll.
There was a problem hiding this comment.
would it make sense to have a Lazy and non lazy version?
There was a problem hiding this comment.
It's common to first hit the DB for an auth check with a value from an authorization header before starting to look at the http body. This seems like a good idea to me.
(based on my understanding that the body would usually be polled immediately, except for use cases where the body is streamed)
Co-authored-by: Daiki Mizukami <tesaguriguma@gmail.com>
| //! Note these middleware differ from [`tower::timeout::Timeout`] which only | ||
| //! adds a timeout to the response future and doesn't consider request bodies. |
There was a problem hiding this comment.
What does this mean for ResponseBodyTimeout? Is it equivalent to the tower middleware?
| enum State { | ||
| NotPolled(Duration), | ||
| SleepPending(#[pin] Sleep), | ||
| } |
There was a problem hiding this comment.
It's common to first hit the DB for an auth check with a value from an authorization header before starting to look at the http body. This seems like a good idea to me.
(based on my understanding that the body would usually be polled immediately, except for use cases where the body is streamed)
| } | ||
| } | ||
|
|
||
| impl<B> Default for TimeoutBody<B> |
There was a problem hiding this comment.
What is this impl useful for?
| @@ -217,13 +218,16 @@ | |||
| clippy::match_like_matches_macro, | |||
| clippy::type_complexity | |||
| )] | |||
| #![forbid(unsafe_code)] | |||
| #![cfg_attr(not(test), forbid(unsafe_code))] | |||
There was a problem hiding this comment.
Seems like all unsafe usage went away in later commits, so this can be reverted?
|
I've been thinking that we probably shouldn't add this to tower-http quite yet. I think it would be better for something like this to be built and mature in a project somewhere and then moved into tower-http later. So I'll close this PR for now. Let me know if you feel this is important. |
|
@davidpdrsn we should add this stuff into tonic and let it mature there maybe? Its quite an important feature for running code in prod. |
|
Sure we could do that! |
This adds two new middleware:
RequestBodyTimeoutResponseBodyTimeoutThey both do pretty much what it says on the tin.