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

reverseproxy, admin: memory leak fix #4482

Merged
merged 2 commits into from
Jan 4, 2022
Merged

reverseproxy, admin: memory leak fix #4482

merged 2 commits into from
Jan 4, 2022

Conversation

dtelyukh
Copy link
Contributor

@francislavoie francislavoie added bug 🐞 Something isn't working under review 🧐 Review is pending before merging labels Dec 16, 2021
@francislavoie francislavoie added this to the v2.5.0 milestone Dec 16, 2021
@mholt
Copy link
Member

mholt commented Dec 23, 2021

Hey thanks for this. I will look at this after the holidays!

return true
case <-ctx.Done():
if !timer.Stop() {
// if the timer has been stopped then read from the channel
<-timer.C

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it might be a good idea to drain the timer in it's own goroutine if the timer.Stop() call doesn't stop the timer. For example something like-

go func(){
	<-timer.C
}()

Also the comment on top of it is not very useful I think. Also, thanks TIL about this memory leak.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so; see the docs: https://pkg.go.dev/time#Timer.Stop

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mholt If the timer has already expired and time.Stop() wouldn't return true and <-timer.C would ensure a value atleast after the duration, but it can be very long too. Draining this in a goroutine would ensure the normal flow of the code from this point of view and draining elsewhere. Please let me know if I am missing something and sorry for being noob in that case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this only reads from the channel if the timer has already been stopped or expired. So the read will never be blocking in this case.

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this contribution -- I suppose I was just lazy, and perhaps this will save some users a little bit of trouble and keep our allocations tidy.

@mholt mholt merged commit 2e46c2a into caddyserver:master Jan 4, 2022
@mholt mholt removed the under review 🧐 Review is pending before merging label Jan 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants