#1314 #1869 #2072 Default timeout enhancements

[hogwartsdeveloper]

Closes #1314 #1869

• Configurable default timeout for HttpMessageInvoker #1314
• Default timeout of HttpMessageInvoker via route configuration #1869

Proposed Changes

• FileGlobalConfiguration add RequestTimeoutSeconds
• MessageInvokerPool use RequestTimeoutSeconds

Discussion

• Unusual spike in response with 499 status code #2072

Successors

• PR #2080 Add new V8 params to fine-tune Polly's circuit-breaker behavior #2081
• TODO: Query regarding `QoSOptions` implemention in version 23.2+ #2112 (comment)

[raman-m]

Thank you for PR creation!

Good start! 🚀

But could you work more plz?
Please pay attention also that you need to implement requirements of both issues: #1314 and #1869

[hogwartsdeveloper]

Thank you for PR creation!

Good start! 🚀

But could you work more plz? Please pay attention also that you need to implement requirements of both issues: #1314 and #1869

Thanks, I'll do this for both tasks

[raman-m]

Please find some time to rebase branch onto develop and I'll review.
Do not forget to Sync fork to have actual develop before rebasing.

[raman-m]

2nd round has finished! My suggestions are below.

[raman-m]

Additionally, we must consider this #2072 (comment). We have an existing Timeout property as shown here:

Ocelot/src/Ocelot/Configuration/File/FileRoute.cs

Line 75 in cc8f5c5

public int Timeout { get; set; }

We need to find a way to repurpose it, as it is particularly pertinent to issue #1869

[hogwartsdeveloper]

Additionally, we must consider this #2072 (comment). We have an existing Timeout property as shown here:

Ocelot/src/Ocelot/Configuration/File/FileRoute.cs

Line 75 in cc8f5c5

public int Timeout { get; set; }

We need to find a way to repurpose it, as it is particularly pertinent to issue #1869

Ok, I found a solution, can you take a look?

[ggnaegi]

Hello, thanks a lot, we should clarify the logic though. In my opinion, we should set a default global timeout value in the FileGlobalConfiguration and handle everything in the ITimeoutCreator implementation.

[raman-m]

I will continue to make suggestions without the expectation of immediate implementation.
We are currently facing design conflicts... 😢

@hogwartsdeveloper, please exercise patience! Our discussions may be lengthy, and delivering both linked features will be challenging. I am optimistic that we will reach a consensus on the current design and necessary refactoring.

At present, it is imperative to include this PR in the current release. The community has been eagerly awaiting the "default timeout feature", which is a high-priority item. Therefore, I will assign this PR to the Spring'24 milestone.

[raman-m]

@hogwartsdeveloper
Here's a summary of today's team discussions:

• Change all int return types to int? for the properties: FileRoute.Timeout, FileGlobalConfiguration.TimeoutSeconds, and FileQoSOptions.TimeoutValue. We've reached a consensus to make them nullable, which will enable us to write cleaner code without gaps in business logic.
• Relocate the ITimeoutCreator.Create method to the IRoutesCreator interface as the CreateTimeout method. The ITimeoutCreator micro-interface seems unnecessary with only one reference and one injection. Moreover, the implementation is one line of code. It will be tested as part of the RoutesCreator class.

These are the essential changes required❗

[ggnaegi]

@raman-m Thanks a a lot @hogwartsdeveloper, I think the QoS issues are not part of this current PR scope, we should address it in a later PR. We are almost there, but it would be wise to use the same property names for the Timeout on the global configuration and the route configuration, with milliseconds precision. Some Acceptance tests are missing (Testing if the Global configuration Timeout is working as expected, that the QoS Timeout value has priority over the global / route Timeout value).

[raman-m]

TODO

• Write major acceptance tests for linked issues → Configurable default timeout for HttpMessageInvoker #1314 and Default timeout of HttpMessageInvoker via route configuration #1869
• Optionally add more unit tests and check code coverage
• Write docs: explain global static default timeout constants, and how to configure global timeouts