Skip to content

Prevent a goroutine leak when healthcheck gets stopped#33781

Merged
aaronlehmann merged 1 commit into
moby:masterfrom
mlaventure:fix-healhcheck-goroutine-leak
Jun 26, 2017
Merged

Prevent a goroutine leak when healthcheck gets stopped#33781
aaronlehmann merged 1 commit into
moby:masterfrom
mlaventure:fix-healhcheck-goroutine-leak

Conversation

@mlaventure

Copy link
Copy Markdown
Contributor

Signed-off-by: Kenfe-Mickael Laventure [email protected]


ping @talex5 I couldn't understand why we don't want to wait for the probe to exit here, so I just put it in a go routine. However, the code is waiting if the context is cancelled a few lines below, can the same be done here?

- What I did

Prevent a go routine leak when a probe has been started and we're asked to stop healthchecks

- How I did it

Started a goroutine that will wait on the channel

- How to verify it

- Description for the changelog

Fix a case were healthcheck could leak a goroutine

- A picture of a cute animal (not mandatory but encouraged)

🐶

@aaronlehmann

Copy link
Copy Markdown

Started a goroutine that will wait on the channel

What about changing the channel to a buffered channel?

@mlaventure

mlaventure commented Jun 22, 2017 via email

Copy link
Copy Markdown
Contributor Author

@talex5

talex5 commented Jun 23, 2017

Copy link
Copy Markdown
Contributor

I don't see any reason why it can't wait either (sorry for the poor comment in the code). Nothing is waiting for monitor() to finish as far as I can see. Looks like making it buffered would be fine too.

@mlaventure
mlaventure force-pushed the fix-healhcheck-goroutine-leak branch from f504a9d to 67297ba Compare June 23, 2017 15:07
@mlaventure

Copy link
Copy Markdown
Contributor Author

Alright, made it a blocking wait.

If we want to take it easy, we could make the channel buffered and not wait, but there's a risk that a few go routine would piles up before the context cancellation gets picked up upon.

@cpuguy83 cpuguy83 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@aaronlehmann

Copy link
Copy Markdown

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants