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(90s) of downstream requests is broken #1833

Closed
huanguolin opened this issue Dec 5, 2023 · 3 comments · Fixed by #1834 or #1847
Closed

Default timeout(90s) of downstream requests is broken #1833

huanguolin opened this issue Dec 5, 2023 · 3 comments · Fixed by #1834 or #1847
Assignees
Labels
bug Identified as a potential bug hotfix Gitflow: Hotfix issue, PR related to hotfix branch merged Issue has been merged to dev and is waiting for the next release QoS Ocelot feature: Quality of Service aka Polly

Comments

@huanguolin
Copy link
Contributor

huanguolin commented Dec 5, 2023

Expected Behavior

According to the documentation, if QoSOptions is not configured, by default, the timeout value for downstream requests is set to 90 seconds. In our project, using Ocelot 18.0.0 + .NET 6, the runtime behavior aligns with the documentation.
image

Actual Behavior

When we upgraded to Ocelot 22.0.0 + .NET 8, also without configuring QoSOptions, the downstream requests never timeout.

Specifications

  • Version: 22.0.0
  • Platform: .NET 8

I took a brief look at the code and noticed that in version 22.0.0, the default value of TimeoutValue in QoSOptions was changed from 0 to int.MaxValue. The code where the default value of 90 seconds is effective seems to be here when it is set to 0. I'm not sure if my analysis is correct.

@raman-m raman-m added bug Identified as a potential bug accepted Bug or feature would be accepted as a PR or is being worked on labels Dec 5, 2023
@raman-m
Copy link
Member

raman-m commented Dec 5, 2023

Hi Alvin!
Thanks for reporting the bug!

Yes, your analysis is correct! The code is not consistent now because the bug was introduced in the default constructor of the FileQoSOptions class.

Will you open a PR with the fix? It should be pretty short and simple to assign zero in default constructor like this

        public FileQoSOptions()
        {
            DurationOfBreak = 1;
            ExceptionsAllowedBeforeBreaking = 0;
            TimeoutValue = 0; // int.MaxValue bug
        }

@raman-m raman-m self-assigned this Dec 5, 2023
@raman-m raman-m added the Nov'23 November 2023 release label Dec 5, 2023
@raman-m raman-m added this to the November'23 milestone Dec 5, 2023
huanguolin pushed a commit to huanguolin/Ocelot that referenced this issue Dec 6, 2023
@huanguolin
Copy link
Contributor Author

@raman-m
Thank you for your prompt response! I'm happy to contribute to the pull request #1834.

huanguolin pushed a commit to huanguolin/Ocelot that referenced this issue Dec 6, 2023
huanguolin pushed a commit to huanguolin/Ocelot that referenced this issue Dec 6, 2023
raman-m pushed a commit to huanguolin/Ocelot that referenced this issue Dec 6, 2023
@raman-m raman-m added hotfix Gitflow: Hotfix issue, PR related to hotfix branch and removed Nov'23 November 2023 release labels Dec 6, 2023
@raman-m raman-m removed this from the November'23 milestone Dec 6, 2023
@raman-m raman-m added the QoS Ocelot feature: Quality of Service aka Polly label Dec 6, 2023
raman-m pushed a commit to huanguolin/Ocelot that referenced this issue Dec 6, 2023
raman-m added a commit that referenced this issue Dec 7, 2023
* fix #1833, Default timeout(90s) of downstream requests is broken

* Fix test Should_log_warning_if_downstream_service_returns_internal_server_error

* Add unit tests

* Recover mixed line endings. Add docs

* Unit tests for HttpClientBuilder

* Add issue info

* Acceptance test

* Just formatting

---------

Co-authored-by: alvin huang <[email protected]>
Co-authored-by: Raman Maksimchuk <[email protected]>
@raman-m raman-m added merged Issue has been merged to dev and is waiting for the next release and removed accepted Bug or feature would be accepted as a PR or is being worked on labels Dec 7, 2023
@raman-m raman-m linked a pull request Dec 8, 2023 that will close this issue
@raman-m raman-m mentioned this issue Dec 8, 2023
@raman-m
Copy link
Member

raman-m commented Dec 8, 2023

Released by #1847

@raman-m raman-m closed this as completed Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Identified as a potential bug hotfix Gitflow: Hotfix issue, PR related to hotfix branch merged Issue has been merged to dev and is waiting for the next release QoS Ocelot feature: Quality of Service aka Polly
Projects
None yet
2 participants