Skip to content

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 :)

@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

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

Development

Successfully merging this pull request may close these issues.

2 participants