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

Timeout on body #303

Merged
merged 16 commits into from Oct 31, 2022
Merged

Timeout on body #303

merged 16 commits into from Oct 31, 2022

Conversation

82marbag
Copy link
Contributor

Motivation

Explained in #295. This PR closes #295.

Solution

Wrap the body in a TimeoutBody. TimeoutBody will poll a sleep future to check whether the body is inactive and register itself to be awoken. The sleep future is polled and checked right after creation to avoid a potential delay in execution making the executor to never poll the sleep future again. That is, if between creation and poll on sleep the time runs out and the sleep is done, a timeout error is immediately returned

Daniele Ahmed and others added 5 commits October 11, 2022 14:56
Closes tower-rs#295
Add a timeout on inactive bodies, both on request and response bodies.

Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
Co-authored-by: Harry Barber <hlbarber@amazon.com>
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
@82marbag
Copy link
Contributor Author

tower-http/src/timeout/body.rs Outdated Show resolved Hide resolved
tower-http/src/timeout/body.rs Outdated Show resolved Hide resolved
tower-http/src/timeout/body.rs Outdated Show resolved Hide resolved
tower-http/src/timeout/body.rs Outdated Show resolved Hide resolved
tower-http/src/timeout/body.rs Outdated Show resolved Hide resolved
tower-http/src/timeout/body.rs Outdated Show resolved Hide resolved
tower-http/src/timeout/body.rs Outdated Show resolved Hide resolved
tower-http/src/timeout/body.rs Outdated Show resolved Hide resolved

pin_project! {
/// Wrapper around a [`http_body::Body`] to time out if data is not ready within the specified duration.
pub struct TimeoutBody<B> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taking a big step back, this could live in http_body crate if there was a standard interface for sleep futures.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think we do want to add a timeout body in http-body-utils, that said we can start with that code in here. I think this is the path I am going with for the retry stuff as well.

Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great start! Left a few comments/questions, lmk if you want to discuss any of it offline.


pin_project! {
/// Wrapper around a [`http_body::Body`] to time out if data is not ready within the specified duration.
pub struct TimeoutBody<B> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think we do want to add a timeout body in http-body-utils, that said we can start with that code in here. I think this is the path I am going with for the retry stuff as well.

tower-http/src/timeout/body.rs Outdated Show resolved Hide resolved
tower-http/src/timeout/body.rs Outdated Show resolved Hide resolved
tower-http/src/timeout/body.rs Outdated Show resolved Hide resolved
tower-http/src/timeout/body.rs Outdated Show resolved Hide resolved
tower-http/src/timeout/body.rs Outdated Show resolved Hide resolved
tower-http/src/timeout/body.rs Outdated Show resolved Hide resolved
tower-http/src/timeout/body.rs Outdated Show resolved Hide resolved
tower-http/src/timeout/body.rs Outdated Show resolved Hide resolved
tower-http/src/timeout/body.rs Outdated Show resolved Hide resolved
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
Daniele Ahmed added 2 commits October 24, 2022 10:53
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
tower-http/src/timeout/body.rs Outdated Show resolved Hide resolved
tower-http/src/timeout/body.rs Outdated Show resolved Hide resolved
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
tower-http/src/timeout/body.rs Outdated Show resolved Hide resolved
tower-http/src/timeout/body.rs Outdated Show resolved Hide resolved
tower-http/src/timeout/body.rs Outdated Show resolved Hide resolved
tower-http/src/timeout/body.rs Outdated Show resolved Hide resolved
tower-http/src/timeout/body.rs Outdated Show resolved Hide resolved
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM just one last thing and we merge

Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SHIP IT

Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
@LucioFranco LucioFranco enabled auto-merge (squash) October 31, 2022 15:06
@LucioFranco LucioFranco merged commit 618a765 into tower-rs:master Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add BodyTimeout middleware
3 participants