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.

Sign up for GitHub

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

Jump to bottom

feat(timeout): add a timeout which can also timeout on poll_ready #677

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

feat(timeout): add a timeout which can also timeout on poll_ready #677

wants to merge 3 commits into from

Conversation

bnjjj
Copy link

@bnjjj bnjjj commented Jul 7, 2022

Following a discussion on Discord I mentioned it was impossible to use Timeout with RateLimit because RateLimit is currently waiting in the poll_ready but the current Timeout implementation doesn't care at all about poll_ready call to inner service. The main goal of this PR is to add the ability to set a timeout for both call to poll_ready and call to be sure we can in case of a wait in poll_ready be able to interrupt/timeout the current request.

I'm not good to find good names so if you have better suggestions than just GlobalTimeout feel free :)

bnjjj added 2 commits July 7, 2022 11:49
@bnjjj
feat(timeout): add a timeout which can also timeout on poll_ready
2c7e8be 
Signed-off-by: Benjamin Coenen <5719034 [email protected]>
@bnjjj
fix dependencies
9f5470f 
Signed-off-by: Benjamin Coenen <5719034 [email protected]>
@bnjjj bnjjj mentioned this pull request Jul 27, 2022
Merged
@tobz
Copy link
Member

tobz commented Jul 21, 2024

I realize this is a two-year old PR, but the sake of reviewing it and providing some feedback...

I can definitely understand the goal here, although this strikes me as kind of a footgun, since a lot of Service implementations treat poll_ready returning an error as a terminal state for the service (i.e. they fuse and can no longer be called). Wrapping the overall call to the service in a timeout would accomplish the same thing, since it can be wrapped around both the waiting for the service to become ready and the call itself.

I think it could potentially be cleaner if this was a helper method on ServiceExt that provided the timeout-wrapped-over-the-poll-ready-call behavior, so it could be named explicitly and make it clear we're waiting specifically for the service to become ready.

@tobz tobz added C-enhancement Category: A PR with an enhancement or a proposed on in an issue. A-timeout Area: The tower "timeout" middleware I-needs-decision Issues in need of decision. T-middleware Topic: middleware labels Jul 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
A-timeout Area: The tower "timeout" middleware C-enhancement Category: A PR with an enhancement or a proposed on in an issue. I-needs-decision Issues in need of decision. T-middleware Topic: middleware
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bnjjj @tobz