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

fix memleak in safe.Pool #6140

Merged
merged 7 commits into from Jan 20, 2020
Merged

fix memleak in safe.Pool #6140

merged 7 commits into from Jan 20, 2020

Conversation

mpl
Copy link
Collaborator

@mpl mpl commented Jan 7, 2020

What does this PR do?

safe.Pool is not a pool in the usual sense as it is not bounded and
it keeps on appending new goroutines, instead of reusing a limited
number of goroutines. Moreover, even when a goroutine is done, there's
still a small footprint of it, as there is a reference to it that was
appended in a slice, and that never gets removed.

This PR fixes the pool by using maps instead of slices, and by making sure the entry in the map corresponding to a goroutine is removed from the map when the goroutine terminates.

Motivation

Fixes #6125

More

  • [ ] Added/updated tests
  • [ ] Added/updated documentation

Additional Notes

Co-authored-by: Julien Salleyron julien.salleyron@gmail.com

@mpl mpl added this to To review in v2 via automation Jan 7, 2020
@traefiker traefiker added this to the 2.1 milestone Jan 7, 2020
Copy link
Contributor

@dtomcej dtomcej left a comment

Choose a reason for hiding this comment

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

LGTM
:shipit:

Copy link
Collaborator

@SantoDE SantoDE left a comment

Choose a reason for hiding this comment

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

LGTM 👼

@ldez ldez self-requested a review January 10, 2020 11:41
@mpl mpl changed the title do not use unbounded routinepool for requests fix memleak in safe.Pool Jan 13, 2020
pkg/safe/routine.go Outdated Show resolved Hide resolved
Copy link
Member

@juliens juliens left a comment

Choose a reason for hiding this comment

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

Nice fix 👏

pkg/safe/routine.go Outdated Show resolved Hide resolved
Copy link
Member

@juliens juliens left a comment

Choose a reason for hiding this comment

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

In fact, I think that the routineCtx slice (or map) is not needed anymore because it was only used for Start mechanism and this mechanism is not used anymore.
I think we should be able to remove all the routines (without Ctx) too, by migrating every old behaviour calls, but this will be done in a future PR.
@mpl I kept the bot/no-merge label for your review, you can merge it when you think it's OK for you.

@mpl mpl removed the bot/no-merge label Jan 20, 2020
@mpl
Copy link
Collaborator Author

mpl commented Jan 20, 2020

In fact, I think that the routineCtx slice (or map) is not needed anymore because it was only used for Start mechanism and this mechanism is not used anymore.
I think we should be able to remove all the routines (without Ctx) too, by migrating every old behaviour calls, but this will be done in a future PR.
@mpl I kept the bot/no-merge label for your review, you can merge it when you think it's OK for you.

LGTM

safe.Pool is not a pool in the usual sense as it is not bounded and
it keeps on appending new goroutines, instead of reusing a limited
number of goroutines. Moreover, even when a goroutine is done, there's
still a small footprint of it, as there is a reference to it that was
appended in a slice, and that never gets removed.

For this reason, it should not be used in places where potentially lots
of go routines are generated, such as in the mirroring service, where
there's one go routine per incoming request.

Fixes traefik#6125
@traefiker traefiker merged commit 24192a3 into traefik:v2.1 Jan 20, 2020
v2 automation moved this from To review to Done Jan 20, 2020
@mpl mpl deleted the fakepool branch January 20, 2020 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v2
Done
Development

Successfully merging this pull request may close these issues.

None yet

7 participants