Skip to content
This repository was archived by the owner on Jul 18, 2025. 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 yudai force-pushed the add_close_to_project branch from 0ef587a to e07b966 Compare March 18, 2017 03:00
@yudai yudai force-pushed the add_close_to_project branch from e07b966 to eb0d286 Compare March 18, 2017 03:01
@yudai yudai force-pushed the add_close_to_project branch 2 times, most recently from 1889c7c to 57ec43a Compare March 20, 2017 21:49
@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
Contributor

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
Contributor

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

@yudai yudai force-pushed the add_close_to_project branch from 57ec43a to a9fa9ac Compare April 25, 2017 17:08
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 yudai force-pushed the add_close_to_project branch from a9fa9ac to 6cce1d7 Compare April 25, 2017 17:12
@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