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

#1724 Reverting back HttpClient full buffering #1853

Conversation

ggnaegi
Copy link
Member

@ggnaegi ggnaegi commented Dec 15, 2023

Follows up #1724

After testing the new http message invoker pool on a test environment, we noticed that some resources were not being released. After further checks, we realised that some streams were not being closed correctly.

This doesn't appear in the application with the standard http client, but I made a mistake by adding the HttpCompletionOption.ResponseHeadersRead property when calling the SendAsync method, which could potentially cause the same problems as with the new message invoker pool.

A revert should therefore be performed for the time being, and this problem will be addressed in the new message invoker pool.

🙈 🙉 🙊

Proposed Changes

  • Reverting back HttpClientWrapper keeping the full buffering for now. Usage of the new option is out of scope and hazardous!!!

… be addressed in the new message invoker pool
@ggnaegi
Copy link
Member Author

ggnaegi commented Dec 15, 2023

@raman-m please, asap ;-)

@raman-m
Copy link
Member

raman-m commented Dec 15, 2023

we noticed that some resources were not being released.

were not being disposed!

@raman-m
Copy link
Member

raman-m commented Dec 15, 2023

HttpCompletionOption.ResponseHeadersRead property

I cannot get how does the property influence to disposing internal resources?
If Yes then .NET framework has bugs... I cannot believe in that.
More realistic scenario is...Ocelot with DI has hidden services which consumes and produces and behaves badly.

@raman-m
Copy link
Member

raman-m commented Dec 15, 2023

I'm not going to approve till Raynald's report if he provides some tests in prod.

@RaynaldM FYI We need your report and approve here.

@raman-m raman-m added this to the November'23 milestone Dec 15, 2023
@raman-m raman-m added the Nov'23 November 2023 release label Dec 15, 2023
@raman-m raman-m changed the title reverting back http client full buffering. It's out of scope and hazardous!!! #1724 Reverting back HttpClient full buffering Dec 15, 2023
@ggnaegi
Copy link
Member Author

ggnaegi commented Dec 15, 2023

HttpCompletionOption.ResponseHeadersRead property

I cannot get how does the property influence to disposing internal resources? If Yes then .NET framework has bugs... I cannot believe in that. More realistic scenario is...Ocelot with DI has hidden services which consumes and produces and behaves badly.

As soon as you decide to handle the stream yourself, then you are responsible of releasing it. It's logical no?

@ggnaegi
Copy link
Member Author

ggnaegi commented Dec 15, 2023

I'm not going to approve till Raynald's report if he provides some tests in prod.

@RaynaldM FYI We need your report and approve here.

@raman-m Raynald has tested a version without these specific changes

@ggnaegi
Copy link
Member Author

ggnaegi commented Dec 15, 2023

we noticed that some resources were not being released.

were not being disposed!

freed willy

@raman-m
Copy link
Member

raman-m commented Dec 15, 2023

I need Ray's input here... There is no problem to revert. The release is still active. This fix is a part of the Nov'23 milestone.

@raman-m raman-m merged commit bb79587 into ThreeMammals:develop Dec 18, 2023
2 checks passed
@RaynaldM
Copy link
Collaborator

I need Ray's input here... There is no problem to revert. The release is still active. This fix is a part of the Nov'23 milestone.

I won't be able to validate these last changes in prod until January 3 or 4.

@raman-m
Copy link
Member

raman-m commented Dec 19, 2023

@RaynaldM 🆗
In this case, it is better to wait new NuGet packs of Nov'23 release and deploy them and test...
So, Dec'23 version will not be ready on January 3...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Nov'23 November 2023 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants