docker / libcompose

*Unmaintained/Deprecated* An experimental go library providing Compose-like functionality
https://godoc.org/github.com/docker/libcompose
Apache License 2.0
585 stars 191 forks source link

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

Open yudai opened 7 years ago

yudai commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

yudai commented 7 years ago

@vdemeester fixed it :)