Skip to content
This repository has been archived by the owner on Jun 16, 2021. It is now read-only.

Add Close() method to Project to release resources #451

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yudai
Copy link
Contributor

@yudai yudai commented Mar 18, 2017

Hi, thank you for the great library.

I found a goroutine leak in the lib, so created a this PR.
It introduces a new method to close a channel, which means it requires a minor API change.

Problem Observed

Goroutine leaks because there is no way to "close" projects.

Problem Detail

NewProject() creates a defaultListener in it.
The listener starts a new goroutine in NewDefaultListener().
We currently don't have a method to stop this goroutine.

Suggested Resolution

This commit adds a new method Close() to release resources
tied to a Project.

@yudai
Copy link
Contributor Author

yudai commented Mar 20, 2017

There is another goroutine leak in the Project. When you create a Project from docker.NewProject() without a custom ClientFactory, it creates a Client from client.NewDefaultFactory(). This factory internally creates a docker client, which requires its Close() to be called to discard the connection to docker dameon.
However, libcompose currently does not call the Close() so connections and goroutines underneath are left behind.
I think we must call the function in the new Close() method as well.

@vdemeester
Copy link
Collaborator

There is another goroutine leak in the Project. When you create a Project from docker.NewProject() without a custom ClientFactory, it creates a Client from client.NewDefaultFactory(). This factory internally creates a docker client, which requires its Close() to be called to discard the connection to docker dameon.

Right

@yudai
Copy link
Contributor Author

yudai commented Mar 22, 2017

It's a bit tricky to fix the leak by the default client factory. Because the factory returns the same instance of APIClient to all Create() calls, so closing the client from a Project causes problems to other Project instances.

Also, APIClient does not expose the Close() method. So it's impossible to close the client from the outside of the factory. I'm wondering if can simply use *client.Client instead of client.APIClient. Or, should we ask docker/docker to add Close() to APIClient?
Another possible way is to define something like APIClientCloser in libcompose/docker. We can also wrap *client.Client with a reference counter.

Here is a workaround with a custom client factory that implements Close(). I'm using this for my project now.
https://gist.github.com/yudai/b1aaf38ed9bbcc1d3717aa853018b811

@vdemeester
Copy link
Collaborator

@yudai ping as it seems there is a build failure 😭

Problem Observed

Goroutine leaks because there is no way to "close" projects

Problem Detail

NewProject() creates a defaultListener in it.
The listener starts a new goroutine in NewDefaultListener().
We currently don't have a method to stop this goroutine.

Suggested Resolution

This commit adds a new method Close() to release resources
tied to a Project.

Signed-off-by: Iwasaki Yudai <[email protected]>
@yudai
Copy link
Contributor Author

yudai commented Apr 25, 2017

@vdemeester fixed it :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants