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

Docker connection not closed #321

Closed
Eun opened this issue May 11, 2021 · 7 comments · Fixed by #939
Closed

Docker connection not closed #321

Eun opened this issue May 11, 2021 · 7 comments · Fixed by #939
Assignees
Labels
hacktoberfest Pull Requests accepted for Hacktoberfest.

Comments

@Eun
Copy link
Contributor

Eun commented May 11, 2021

Describe the bug
The ProviderType.GetProvider() function creates a docker client, but never closes it, which leads to a leaking connection.

Similar to #319

To Reproduce
Use following test:

package main

import (
	"context"
	"testing"
	"time"

	"github.com/testcontainers/testcontainers-go"
	"github.com/testcontainers/testcontainers-go/wait"
	"go.uber.org/goleak"
)

func TestMain(m *testing.M) {
	goleak.VerifyTestMain(m)
}

func TestFoo(t *testing.T) {
	ctx, cancel := context.WithTimeout(context.Background(), time.Minute)
	defer cancel()
	req := testcontainers.ContainerRequest{
		Image:        "postgres:12",
		ExposedPorts: []string{"5432/tcp"},
		AutoRemove:   true,
		Env: map[string]string{
			"POSTGRES_USER":     "postgres",
			"POSTGRES_PASSWORD": "postgres",
			"POSTGRES_DB":       "postgres",
		},
		WaitingFor: wait.ForListeningPort("5432/tcp"),
	}
	container, err := testcontainers.GenericContainer(ctx, testcontainers.GenericContainerRequest{
		ContainerRequest: req,
		Started:          true,
	})
	if err != nil {
		panic(err)
	}
	if err := container.Terminate(ctx); err != nil {
		panic(err)
	}
}

Expected behavior
A successful test

docker info
output of the command:

$ docker info
Client:
 Context:    default
 Debug Mode: false

Server:
 Containers: 18
  Running: 1
  Paused: 0
  Stopped: 17
 Images: 72
 Server Version: 20.10.2
 Storage Driver: zfs
  Zpool: rpool
  Zpool Health: ONLINE
  Parent Dataset: rpool/var/lib/docker
  Space Used By Parent: 10480205824
  Space Available: 145392234496
  Parent Quota: no
  Compression: lz4
 Logging Driver: json-file
 Cgroup Driver: cgroupfs
 Cgroup Version: 1
 Plugins:
  Volume: local
  Network: bridge host ipvlan macvlan null overlay
  Log: awslogs fluentd gcplogs gelf journald json-file local logentries splunk syslog
 Swarm: inactive
 Runtimes: io.containerd.runc.v2 io.containerd.runtime.v1.linux runc
 Default Runtime: runc
 Init Binary: docker-init
 containerd version: 
 runc version: 
 init version: 
 Security Options:
  apparmor
  seccomp
   Profile: default
 Kernel Version: 5.8.0-50-generic
 Operating System: Ubuntu 20.04.2 LTS
 OSType: linux
 Architecture: x86_64
 CPUs: 8
 Total Memory: 15.53GiB
 Name: tobias-talon-T480s
 ID: UMBP:HB2G:5S3P:JMX3:VZ46:ATMB:64RC:4WXO:YJ6L:M7UK:DWGB:JJ25
 Docker Root Dir: /var/lib/docker
 Debug Mode: false
 Registry: https://index.docker.io/v1/
 Labels:
 Experimental: false
 Insecure Registries:
  127.0.0.0/8
 Live Restore Enabled: false

Additional context
It might be necessary to adjust GenericProvider to include a Close() function that closes the used provider.

@mdelapenya
Copy link
Collaborator

mdelapenya commented May 11, 2021

Hi @Eun, thanks for reporting this issue. Would you like to send a PR with this implementation? We'll be fast to review it.

If not, I'll find time to implement it and ask for your review when I have bandwidth to do it.

Thanks in advance!!

@mdelapenya
Copy link
Collaborator

Hi @Eun I'm not able to reproduce this bug yet, using the same repro code shared here.

Could you please double check if it's still happening on your side? 🙏

@mdelapenya mdelapenya self-assigned this Sep 15, 2022
@mdelapenya
Copy link
Collaborator

OTOH let me rephrase it myself: do you think all the usages of the provider.client should automatically Close the connection? 🤔

@Eun
Copy link
Contributor Author

Eun commented Sep 16, 2022

I cannot reproduce either! Could be related to new versions of goleak or go or whatever.

However I can remember the case:

In docker.go a new docker.Client instance gets created in func NewDockerProvider() (*DockerProvider, error).

However, this instance will never be closed.
It should be closed calling client.Close
I think the only solution is to add a Close function to the GenericProvider interface and call it after being done.

@mdelapenya
Copy link
Collaborator

Would you mind elaborating a PR? Otherwise, we can prioritize it for our upcoming iterations

Cheers!

@Eun
Copy link
Contributor Author

Eun commented Sep 18, 2022

I am kinda busy at this moment, so wont make it happen.

@AlexanderYastrebov
Copy link
Contributor

See also related #1357

AlexanderYastrebov added a commit to AlexanderYastrebov/testcontainers-go that referenced this issue Jul 10, 2023
Fixes various goroutine leaks by moving around and adding missing Close calls.

Leaks were detected by github.com/AlexanderYastrebov/noleak by adding

```go
// go get github.com/AlexanderYastrebov/noleak@latest
// cat main_test.go
package testcontainers

import (
        "os"
        "testing"

        "github.com/AlexanderYastrebov/noleak"
)

func TestMain(m *testing.M) {
        os.Exit(noleak.CheckMain(m))
}
```

Not all leaks could be fixed e.g. due to testcontainers#1357 therefore this change
does not add leak detector, only fixes.

Updates testcontainers#321
AlexanderYastrebov added a commit to AlexanderYastrebov/testcontainers-go that referenced this issue Jul 12, 2023
Fixes various goroutine leaks by moving around and adding missing Close calls.

Leaks were detected by github.com/AlexanderYastrebov/noleak by adding

```go
// go get github.com/AlexanderYastrebov/noleak@latest
// cat main_test.go
package testcontainers

import (
        "os"
        "testing"

        "github.com/AlexanderYastrebov/noleak"
)

func TestMain(m *testing.M) {
        os.Exit(noleak.CheckMain(m))
}
```

Not all leaks could be fixed e.g. due to testcontainers#1357 therefore this change
does not add leak detector, only fixes.

Updates testcontainers#321
AlexanderYastrebov added a commit to AlexanderYastrebov/testcontainers-go that referenced this issue Aug 5, 2023
Fixes various goroutine leaks by moving around and adding missing Close calls.

Leaks were detected by github.com/AlexanderYastrebov/noleak by adding

```go
// go get github.com/AlexanderYastrebov/noleak@latest
// cat main_test.go
package testcontainers

import (
        "os"
        "testing"

        "github.com/AlexanderYastrebov/noleak"
)

func TestMain(m *testing.M) {
        os.Exit(noleak.CheckMain(m))
}
```

Not all leaks could be fixed e.g. due to testcontainers#1357 therefore this change
does not add leak detector, only fixes.

Updates testcontainers#321
AlexanderYastrebov added a commit to AlexanderYastrebov/testcontainers-go that referenced this issue Aug 5, 2023
Fixes various goroutine leaks by moving around and adding missing Close calls.

Leaks were detected by github.com/AlexanderYastrebov/noleak by adding

```go
// go get github.com/AlexanderYastrebov/noleak@latest
// cat main_test.go
package testcontainers

import (
        "os"
        "testing"

        "github.com/AlexanderYastrebov/noleak"
)

func TestMain(m *testing.M) {
        os.Exit(noleak.CheckMain(m))
}
```

Not all leaks could be fixed e.g. due to testcontainers#1357 therefore this change
does not add leak detector, only fixes.

Updates testcontainers#321
AlexanderYastrebov added a commit to AlexanderYastrebov/testcontainers-go that referenced this issue Aug 6, 2023
Fixes various goroutine leaks by moving around and adding missing Close calls.

Leaks were detected by github.com/AlexanderYastrebov/noleak by adding

```go
// go get github.com/AlexanderYastrebov/noleak@latest
// cat main_test.go
package testcontainers

import (
        "os"
        "testing"

        "github.com/AlexanderYastrebov/noleak"
)

func TestMain(m *testing.M) {
        os.Exit(noleak.CheckMain(m))
}
```

Not all leaks could be fixed e.g. due to testcontainers#1357 therefore this change
does not add leak detector, only fixes.

Updates testcontainers#321
AlexanderYastrebov added a commit to AlexanderYastrebov/testcontainers-go that referenced this issue Aug 6, 2023
Fixes various goroutine leaks by moving around and adding missing Close calls.

Leaks were detected by github.com/AlexanderYastrebov/noleak by adding

```go
// go get github.com/AlexanderYastrebov/noleak@latest
// cat main_test.go
package testcontainers

import (
        "os"
        "testing"

        "github.com/AlexanderYastrebov/noleak"
)

func TestMain(m *testing.M) {
        os.Exit(noleak.CheckMain(m))
}
```

Not all leaks could be fixed e.g. due to testcontainers#1357 therefore this change
does not add leak detector, only fixes.

Updates testcontainers#321
mdelapenya pushed a commit that referenced this issue Aug 9, 2023
Fixes various goroutine leaks by moving around and adding missing Close calls.

Leaks were detected by github.com/AlexanderYastrebov/noleak by adding

```go
// go get github.com/AlexanderYastrebov/noleak@latest
// cat main_test.go
package testcontainers

import (
        "os"
        "testing"

        "github.com/AlexanderYastrebov/noleak"
)

func TestMain(m *testing.M) {
        os.Exit(noleak.CheckMain(m))
}
```

Not all leaks could be fixed e.g. due to #1357 therefore this change
does not add leak detector, only fixes.

Updates #321
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest Pull Requests accepted for Hacktoberfest.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants