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

Default timeout of HttpMessageInvoker via route configuration #1869

Open
RightDecision2 opened this issue Dec 26, 2023 · 16 comments · May be fixed by #2073
Open

Default timeout of HttpMessageInvoker via route configuration #1869

RightDecision2 opened this issue Dec 26, 2023 · 16 comments · May be fixed by #2073
Assignees
Labels
Configuration Ocelot feature: Configuration proposal Proposal for a new functionality in Ocelot QoS Ocelot feature: Quality of Service Spring'24 Spring 2024 release
Milestone

Comments

@RightDecision2
Copy link

RightDecision2 commented Dec 26, 2023

Expected Behavior / New Feature

_defaultTimeout = TimeSpan.FromSeconds(_configuration.TimeOut);

Actual Behavior / Motivation for New Feature

_defaultTimeout = TimeSpan.FromSeconds(90);

In MessageInvokerPool:

public const int DefaultRequestTimeoutSeconds = 90;

and
// Adding timeout handler to the top of the chain.
// It's standard behavior to throw TimeoutException after the defined timeout (90 seconds by default)
var timeoutHandler = new TimeoutDelegatingHandler(downstreamRoute.QosOptions.TimeoutValue == 0
? TimeSpan.FromSeconds(RequestTimeoutSeconds)
: TimeSpan.FromMilliseconds(downstreamRoute.QosOptions.TimeoutValue))

in version 23.0

/// <summary>
/// TODO This should be configurable and available as global config parameter in ocelot.json.
/// </summary>
public const int DefaultRequestTimeoutSeconds = 90;

Specifications

  • Version: 23.0.0+
  • Platform: All
  • Subsystem: Any
@raman-m
Copy link
Member

raman-m commented Dec 26, 2023

Are you going to create a PR these days?

@RightDecision2
Copy link
Author

RightDecision2 commented Dec 26, 2023 via email

@raman-m
Copy link
Member

raman-m commented Dec 26, 2023

What is the problem with default 90 seconds timeout?

As a quick fix use QoS TimeoutValue property to have custom timeout value.
I don't think that performance will be decreased dramatically: a few microseconds will be added 😄

@raman-m
Copy link
Member

raman-m commented Dec 26, 2023

@RightDecision2 commented on Dec 26

👌 Great! Waiting for your PR creation... Will review with pleasure!
But you need to fork Ocelot repository first!
Your repos are empty now.

@raman-m raman-m added QoS Ocelot feature: Quality of Service Configuration Ocelot feature: Configuration proposal Proposal for a new functionality in Ocelot labels Dec 26, 2023
@raman-m raman-m changed the title Add to configuration hardcoded timeout of HttpClient Add HttpClient timeout to route configuration Dec 26, 2023
@raman-m raman-m changed the title Add HttpClient timeout to route configuration Default timeout of HttpClient via route configuration Dec 26, 2023
@RightDecision2
Copy link
Author

RightDecision2 commented Dec 27, 2023 via email

@raman-m
Copy link
Member

raman-m commented Dec 27, 2023

Dear author, What's your name?
What's your LinkedIn?

@raman-m
Copy link
Member

raman-m commented Dec 27, 2023

I have OCR service, which process pdf file. Every request average 2mins.
When I use QoS its random cancelled after 90 seconds (few times might be worked).

Strange cancellation of your requests by QoS logic... You need to show this behavior by sharing to us Ocelot logs.
It should not be cancelled randomly... All requests should be done without problems if TimeoutValue in QoS is greater than 2 mins (average processing time by your downstream service). If some requests failed, that means root cause is not on Ocelot's side...

What is the maximum processing time? Seems you have to define max value in route config.
What exact value of TimeoutValue?

Please attach route JSON!

@raman-m
Copy link
Member

raman-m commented Dec 27, 2023

So for me, is only one possible solution - increase default timeout. I think for someone it could be useful to.

I'm not sure it will help you...
How stable your downstream service if you connect directly (without Ocelot routing)?

As we've agreed on, you are going to create a PR soon to manage default timeout from route config, right?
Could you Sync fork please?
Our development process requires to fork the Ocelot repo first.

@raman-m raman-m added the low Low priority label Jan 7, 2024
@RightDecision2
Copy link
Author

RightDecision2 commented Jan 9, 2024 via email

@lemosluan
Copy link

lemosluan commented Feb 8, 2024

The doc from ocelot's site says that there is a timeout to 90s.
You can use Polly to reach a new timeout, but in some cases, the response from our services take more than 90s, so it would be useful if the timeout on startup of ocelot can be set.

image

@raman-m
Copy link
Member

raman-m commented Feb 22, 2024

@RightDecision2 commented on Jan 9

How is your development going?
I guess you should be interested in fixing your problem...
But sorry, as a team, we are busy with other more important tasks. So, you need to fix by your own and create a PR, if you wish to contribute.

Will you contribute or not?

Можешь писать на русском языке, если это ускорит нашу разработку.
Не люблю, когда скрываются под всякими анонимными никами, да ещё и отвечают с мобилы, где установлен русскоязычный почтовый клиент.
И я задавал этот вопрос
Зачем шифроваться под левой учёткой ГитХаба? Не понимаю этого! Наверняка есть основной аккаунт, другой...

@raman-m
Copy link
Member

raman-m commented Feb 22, 2024

@lemosluan commented on Feb 8

Hi Luan!
What's your solution for Ocelot pipeline setup?

This author's problem can be fixed by a Delegating Handler.
A few lines of code in the body of the handler. Something like that:

public class TimeoutHandler : DelegatingHandler
{
    protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken token)
    {
            TimeSpan? timeout = TimeSpan.FromHours(1.0); // hardcoded but can be read from the config
            request.Properties["RequestTimeout"] = timeout;
            return await base.SendAsync(request, token);
    }
}

But I'm not sure on design correctness, it should be checked.
Personally I like the following robust solution: Better timeout handling with HttpClient

And after that, just attach TimeoutHandler to Ocelot pipeline as:

ConfigureServices(s => s
    .AddOcelot()
    .AddDelegatingHandler<TimeoutHandler>()
)

and route config is

{
  // other props
  "DelegatingHandlers": ["TimeoutHandler"]
}

@RightDecision2
Copy link
Author

RightDecision2 commented Feb 23, 2024 via email

@raman-m
Copy link
Member

raman-m commented Feb 23, 2024

@RightDecision2 Приоритет низкий!
Без понятия в какой релиз это попадёт. Если кто-то создаст ПР, то фича появится быстрее.
Ждите! Может кто-то контрибьютнет...

@hogwartsdeveloper
Copy link
Contributor

I would like to do it.

@raman-m
Copy link
Member

raman-m commented May 26, 2024

I would like to do it.

@hogwartsdeveloper Dear Zhannur,
You are assigned!

@raman-m raman-m linked a pull request May 26, 2024 that will close this issue
@raman-m raman-m added Spring'24 Spring 2024 release and removed low Low priority labels May 29, 2024
@raman-m raman-m added this to the Spring'24 milestone May 29, 2024
@raman-m raman-m changed the title Default timeout of HttpClient via route configuration Default timeout of HttpMessageInvoker via route configuration May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Configuration Ocelot feature: Configuration proposal Proposal for a new functionality in Ocelot QoS Ocelot feature: Quality of Service Spring'24 Spring 2024 release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants