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

Provide a way to limit the total duration of a client request #4591

Closed
jrhee17 opened this issue Dec 27, 2022 · 1 comment · Fixed by #5793
Closed

Provide a way to limit the total duration of a client request #4591

jrhee17 opened this issue Dec 27, 2022 · 1 comment · Fixed by #5793
Milestone

Comments

@jrhee17
Copy link
Contributor

jrhee17 commented Dec 27, 2022

Background

Currently, client requests are limited by a series of cancellation schedulers.

An example could be as follows:

  1. A request tries to initialize the endpoint
  2. A request invokes a dns query
  3. A connection is acquired for a request
  4. A request writes to a connection
  5. A request waits for the response

Each of the above steps are limited by a timeout (there is also an idle timeout which can be triggered between steps).

If a user would like to limit a request to X seconds, there is no easy way to do so beside tuning all relevant parameters.

Proposal

It has been proposed that it might be convenient for users if there was an ability to limit the total duration of a client request.

More specifically, it might be useful if users were able to specify the start time of the responseTimeout as an option.

There has been a couple of points from internal discussion:

  • The totalTimeout should have the option to start from before decorator execution (so when ClientRequestContext is created)
    • If a timeout is invoked, ideally the decorator chain will halt execution.
  • It might be possible to generalize if we give a ClientOption to specify the when to start the responseTimeout. (e.g. before dns, before connection, etc..)
    • We should take care to throw the appropriate exception depending on which stage the timeout has been invoked.
  • Should we also generalize when the timeout ends?
    • The timing may be different between 1) when the response is read from the socket and parsed into a http response 2) when the returned http response is consumed by the client
    • It might be a good idea to cancel the cancellation scheduler when deserialization is complete (grpc, thrift, etc..).

Reference: https://line-armeria.slack.com/archives/C1NGPBUH2/p1672103603661809

@jrhee17 jrhee17 added this to the 1.30.0 milestone Jun 27, 2024
@jrhee17
Copy link
Contributor Author

jrhee17 commented Jun 27, 2024

More specifically, it might be useful if users were able to specify the start time of the responseTimeout as an option.

Let me try to add a POC for this implementation.

I guess the new API may look something like the following:

enum ResponseTimeoutStartTiming {
    CLIENT_EXECUTE,
    REQUEST_WRITE,
    RESPONSE_WRITE,
}

public AbstractClientOptionsBuilder startResponseTimeout(ResponseTimeoutStartTiming startTiming) {
    return this;
}

We may also add the analogous APIs to ClientFactory and Flags.

@ikhoon ikhoon modified the milestones: 1.30.0, 1.31.0 Aug 1, 2024
jrhee17 added a commit that referenced this issue Aug 9, 2024
…ely (#5800)

Motivation:

In order to handle #4591, I
propose that we first define an API which allows users to cancel a
request. Currently, `ClientRequestContext#cancel` is invoked once
`CancellationScheduler#start` is invoked. I propose to change the
behavior of `ClientRequestContext#cancel` such that the associated
request is aborted immediately. Once this API is available, it would be
trivial to implement `ResponseTimeoutMode` by adjusting where to call
`CancellationScheduler#start`. Additionally, users may easily implement
their own timeout mechanism if they would like.

Modifications:

- `CancellationScheduler` related changes
- `DefaultCancellationScheduler` is refactored to be lock based instead
of event loop based. The reasoning for this change was for the scenario
where the request execution didn't reach the event loop yet. i.e. If a
user calls `ctx.cancel` in the decorator, a connection shouldn't be
opened.
- e.g.
https://github.com/line/armeria/blob/59aa40a59e1f1122716e70f9f1d6f1402a6aae0e/core/src/test/java/com/linecorp/armeria/client/ContextCancellationTest.java#L90-L116
- `CancellationScheduler#updateTask` is introduced. This API updates the
cancellation task if the scheduler isn't invoked yet. If the scheduler
is invoked already, the cancellation task will be executed eventually.
This API allows `ctx.cancel` to attempt cancellation depending on which
stage the request is at. For instance, at the decorators only
`req.abort` needs to be called but at the request write stage, the
cancellation task may need to send a reset signal.
- Misc. an infinite timeout is internally represented as
`Long.MAX_VALUE` instead of `0`
- `AbstractHttpRequestHandler` related changes
- `CancellationTask` in `AbstractHttpRequestHandler`,
`HttpResponseWrapper`, `AbstractHttpResponseHandler` is modified to be
scheduled inside an event loop. The reasoning is that `ctx.cancel`, and
hence `CancellationTask#run` can be invoked from any thread.
- There is a higher chance of `AbstractHttpRequestHandler` calling
`fail` or `failAndReset` multiple times. There is no point in doing so,
so added a boolean flag `failed` for safety.
- `HttpResponseWrapper` related changes
- The original intention of `cancelTimeoutAndLog` was to not log if the
response is unexpected. Modified so that if the response is cancelled or
the context is cancelled, no logging is done
- There is probably no reason to not call `close` when a timeout occurs.
Unified the fragmented logic of closing the `HttpResponseWrapper`.

Result:

- Users can call `ClientRequestContext#cancel` to cancel the ongoing
request easily.
- #5793 can be prepared for

<!--
Visit this URL to learn more about how to write a pull request
description:

https://armeria.dev/community/developer-guide#how-to-write-pull-request-description
-->

---------

Co-authored-by: Ikhun Um <[email protected]>
@ikhoon ikhoon closed this as completed in 7c21957 Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants