search

ThreeMammals / Ocelot

.NET API Gateway
https://www.nuget.org/packages/Ocelot
MIT License
8.31k stars 1.63k forks source link

#1724 Reverting back HttpClient full buffering #1853

Closed ggnaegi closed 8 months ago

ggnaegi commented 9 months ago

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

ggnaegi commented 9 months ago

@raman-m please, asap ;-)

raman-m commented 9 months ago

we noticed that some resources were not being released.

were not being disposed!

raman-m commented 9 months ago

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 commented 9 months ago

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.

ggnaegi commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

we noticed that some resources were not being released.

were not being disposed!

freed willy

raman-m commented 9 months ago

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.

RaynaldM commented 8 months ago

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 commented 8 months ago

@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...