Skip to content

Prevent timer leaks in Workerpool and others#11333

Merged
zeripath merged 4 commits into
go-gitea:masterfrom
zeripath:prevent-leak-workerpool-timers
May 8, 2020
Merged

Prevent timer leaks in Workerpool and others#11333
zeripath merged 4 commits into
go-gitea:masterfrom
zeripath:prevent-leak-workerpool-timers

Conversation

@zeripath
Copy link
Copy Markdown
Contributor

@zeripath zeripath commented May 8, 2020

There is a memory leak in Workerpool due to the intricacies of
time.Timer stopping.

Whenever a time.Timer is Stopped its channel must be cleared using a
select if the result of the Stop() is false.

Unfortunately in Workerpool these were checked the wrong way round.

However, there were a few other places that were not being checked.

Signed-off-by: Andrew Thornton art27@cantab.net

There is a memory leak in `Workerpool` due to the intricacies of
`time.Timer` stopping.

Whenever a `time.Timer` is `Stop`ped its channel must be cleared using a
`select` if the result of the `Stop()` is `false`.

Unfortunately in `Workerpool` these were checked the wrong way round.

However, there were a few other places that were not being checked.

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath added type/bug issue/critical This issue should be fixed ASAP. If it is a PR, the PR should be merged ASAP backport/v1.11 labels May 8, 2020
@zeripath zeripath added this to the 1.12.0 milestone May 8, 2020
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label May 8, 2020
Signed-off-by: Andrew Thornton <art27@cantab.net>
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 8, 2020
@6543
Copy link
Copy Markdown
Member

6543 commented May 8, 2020

🚀

@kevans91
Copy link
Copy Markdown
Contributor

kevans91 commented May 8, 2020

I suspect some of these drains might be able to go away to simplify things -- according to https://sourcegraph.com/github.com/golang/go/-/commit/ba3149612f62c011765876d7a437095fa50e0771 and some clarification on IRC, draining the channel isn't to prevent a leak within the channel but to attempt prevention of a false-positive on a consumer of the timer. So the two situations you'd actually be required to drain it are:

1.) If you're going to subsequently Reset() the timer, or
2.) If anyone waiting on the channel might be sensitive to the timer firing just before you stopped it

I think the majority of these consumers fall into neither, as the timer's oftentimes only consumed by the single channel and not getting re-used.

@zeripath
Copy link
Copy Markdown
Contributor Author

zeripath commented May 8, 2020

make lg-tm work

@zeripath zeripath merged commit c58bc4b into go-gitea:master May 8, 2020
@zeripath zeripath deleted the prevent-leak-workerpool-timers branch May 8, 2020 15:46
zeripath added a commit to zeripath/gitea that referenced this pull request May 8, 2020
There is a potential memory leak in `Workerpool` due to the intricacies of
`time.Timer` stopping.

Whenever a `time.Timer` is `Stop`ped its channel must be cleared using a
`select` if the result of the `Stop()` is `false`.

Unfortunately in `Workerpool` these were checked the wrong way round.

However, there were a few other places that were not being checked.

Signed-off-by: Andrew Thornton <art27@cantab.net>

Co-authored-by: techknowlogick <techknowlogick@gitea.io>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
lunny added a commit that referenced this pull request May 8, 2020
There is a potential memory leak in `Workerpool` due to the intricacies of
`time.Timer` stopping.

Whenever a `time.Timer` is `Stop`ped its channel must be cleared using a
`select` if the result of the `Stop()` is `false`.

Unfortunately in `Workerpool` these were checked the wrong way round.

However, there were a few other places that were not being checked.

Signed-off-by: Andrew Thornton <art27@cantab.net>

Co-authored-by: techknowlogick <techknowlogick@gitea.io>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>

Co-authored-by: techknowlogick <techknowlogick@gitea.io>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
ydelafollye pushed a commit to ydelafollye/gitea that referenced this pull request Jul 31, 2020
There is a potential memory leak in `Workerpool` due to the intricacies of
`time.Timer` stopping.

Whenever a `time.Timer` is `Stop`ped its channel must be cleared using a
`select` if the result of the `Stop()` is `false`.

Unfortunately in `Workerpool` these were checked the wrong way round.

However, there were a few other places that were not being checked.

Signed-off-by: Andrew Thornton <art27@cantab.net>

Co-authored-by: techknowlogick <techknowlogick@gitea.io>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

issue/critical This issue should be fixed ASAP. If it is a PR, the PR should be merged ASAP lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants