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

HTTP response body not always closed #18

Closed
freedge opened this issue Apr 12, 2024 · 5 comments
Closed

HTTP response body not always closed #18

freedge opened this issue Apr 12, 2024 · 5 comments

Comments

@freedge
Copy link

freedge commented Apr 12, 2024

same as heroku/docker-registry-client#98

the following code
https://github.com/stackrox/docker-registry-client/blob/master/registry/tokentransport.go#L74-L77
fails to close the body if response.StatusCode != http.StatusOK and a socket is leaked

@RTann
Copy link
Collaborator

RTann commented Apr 12, 2024

Thank you for bringing this to our attention! I created #19 in hopes to address this.

We also do not actively work on this, and we are in the process of switching over to https://github.com/google/go-containerregistry. That repo is actively maintained, though it does not support Docker Schema 1 (though nobody should really be using that anymore, anyway). If possible, I'd recommend using that or some other repo which is more actively maintained than this one.

@freedge
Copy link
Author

freedge commented Apr 13, 2024

hi thanks for the response! well feel free to reject this ticket. While I am using heroku's docker registry client (and found sockets going up the roof) I was mostly intrigued with stackrox usage of it. If you're switching to a better client then I'm not too concerned anymore. Cheers!

@RTann
Copy link
Collaborator

RTann commented Apr 17, 2024

I merged #19 though when looking through it some more, I realized that this repo is not affected by the memory leak due to https://github.com/stackrox/docker-registry-client/blob/main/registry/tokentransport.go#L52 which was introduced by #7. Because of this, #19 now just makes it clear the returned err == nil and it also fixes an unrelated potential panic

@RTann
Copy link
Collaborator

RTann commented Apr 17, 2024

If you agree with ^, then I will close this issue. Thanks!

@freedge
Copy link
Author

freedge commented Apr 18, 2024

awesome, Thanks!

@freedge freedge closed this as completed Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants