Colin-b / httpx_auth

Authentication classes to be used with httpx
MIT License
114 stars 26 forks source link

Fix a memory leak in repeated AWS4Auth initializations #68

Closed miikka closed 7 months ago

miikka commented 10 months ago

This PR fixes the issue #67. Before the change creating AWS4Auth instances with security_token set would add one entry per instance to default_include_headers. This PR fixes it by always copying the list before modifying it.

sonarcloud[bot] commented 10 months ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Colin-b commented 10 months ago

Hello @miikka

Thanks for the proposal. Looking at the code I think a more elegant solution would be to address the TODO in the code base in the first place. This variable should indeed not be shared accross instances. And in addition it might make more sense to use a set here. I will take care of this during the weekend.

Thanks again

miikka commented 10 months ago

Makes sense. In this PR, I tried to do the minimal change to address the issue, but addressing both TODOs properly would certainly be better. Looking forward to your solution.

marioplumbarius commented 7 months ago

Hello @miikka

Thanks for the proposal. Looking at the code I think a more elegant solution would be to address the TODO in the code base in the first place. This variable should indeed not be shared accross instances. And in addition it might make more sense to use a set here. I will take care of this during the weekend.

Thanks again

Any progress here?

Colin-b commented 7 months ago

For now I am focusing on improving the test suite so that we can work on proper async support. Once test suite is ready however I plan on tackling those "quick" win.

sonarcloud[bot] commented 7 months ago

Quality Gate Failed Quality Gate failed

Failed conditions

28.8% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

Colin-b commented 7 months ago

Thanks again for the proposal, I will merge and update to properly address todo

Colin-b commented 7 months ago

Release 0.20.0 is now available on pypi with this fix (amongst others).