Skip to content

Commit

Permalink
all: fix goroutine leaks
Browse files Browse the repository at this point in the history
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
  • Loading branch information
AlexanderYastrebov committed Jul 12, 2023
1 parent f5a4a54 commit 3febcdc
Show file tree
Hide file tree
Showing 7 changed files with 67 additions and 19 deletions.
47 changes: 31 additions & 16 deletions docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,14 @@ func (c *DockerContainer) Stop(ctx context.Context, timeout *time.Duration) erro

// Terminate is used to kill the container. It is usually triggered by as defer function.
func (c *DockerContainer) Terminate(ctx context.Context) error {
select {
// close reaper if it was created
case c.terminationSignal <- true:
default:
}

defer c.provider.client.Close()

err := c.terminatingHook(ctx)
if err != nil {
return err
Expand All @@ -255,11 +263,6 @@ func (c *DockerContainer) Terminate(ctx context.Context) error {
return err
}

select {
// close reaper if it was created
case c.terminationSignal <- true:
default:
}
err = c.provider.client.ContainerRemove(ctx, c.GetContainerID(), types.ContainerRemoveOptions{
RemoveVolumes: true,
Force: true,
Expand All @@ -283,33 +286,29 @@ func (c *DockerContainer) Terminate(ctx context.Context) error {
}
}

if err := c.provider.client.Close(); err != nil {
return err
}

c.sessionID = uuid.UUID{}
c.isRunning = false
return nil
}

// update container raw info
func (c *DockerContainer) inspectRawContainer(ctx context.Context) (*types.ContainerJSON, error) {
defer c.provider.Close()
inspect, err := c.provider.client.ContainerInspect(ctx, c.ID)
if err != nil {
return nil, err
}
defer c.provider.Close()

c.raw = &inspect
return c.raw, nil
}

func (c *DockerContainer) inspectContainer(ctx context.Context) (*types.ContainerJSON, error) {
defer c.provider.Close()
inspect, err := c.provider.client.ContainerInspect(ctx, c.ID)
if err != nil {
return nil, err
}
defer c.provider.Close()

return &inspect, nil
}
Expand Down Expand Up @@ -739,13 +738,9 @@ func (n *DockerNetwork) Remove(ctx context.Context) error {
default:
}

err := n.provider.client.NetworkRemove(ctx, n.ID)
if err != nil {
return err
}
defer n.provider.Close()

return nil
return n.provider.client.NetworkRemove(ctx, n.ID)
}

// DockerProvider implements the ContainerProvider interface
Expand Down Expand Up @@ -913,6 +908,13 @@ func (p *DockerProvider) CreateContainer(ctx context.Context, req ContainerReque
}
}

// Cleanup on error, otherwise set termSignal to nil before successful return.
defer func() {
if termSignal != nil {
termSignal <- true
}
}()

if err = req.Validate(); err != nil {
return nil, err
}
Expand Down Expand Up @@ -1091,6 +1093,9 @@ func (p *DockerProvider) CreateContainer(ctx context.Context, req ContainerReque
return nil, err
}

// Disable cleanup on success
termSignal = nil

return c, nil
}

Expand Down Expand Up @@ -1306,6 +1311,13 @@ func (p *DockerProvider) CreateNetwork(ctx context.Context, req NetworkRequest)
}
}

// Cleanup on error, otherwise set termSignal to nil before successful return.
defer func() {
if termSignal != nil {
termSignal <- true
}
}()

response, err := p.client.NetworkCreate(ctx, req.Name, nc)
if err != nil {
return &DockerNetwork{}, err
Expand All @@ -1319,6 +1331,9 @@ func (p *DockerProvider) CreateNetwork(ctx context.Context, req NetworkRequest)
provider: p,
}

// Disable cleanup on success
termSignal = nil

return n, nil
}

Expand Down
1 change: 1 addition & 0 deletions docker_auth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ func removeImageFromLocalCache(t *testing.T, image string) {
if err != nil {
t.Log("could not create client to cleanup registry: ", err)
}
defer testcontainersClient.Close()

_, err = testcontainersClient.ImageRemove(ctx, image, types.ImageRemoveOptions{
Force: true,
Expand Down
33 changes: 30 additions & 3 deletions docker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -200,9 +200,11 @@ func TestContainerWithHostNetworkOptions_UseExposePortsFromImageConfigs(t *testi
t.Errorf("Expected server endpoint. Got '%v'.", err)
}

_, err = http.Get(endpoint)
resp, err := http.Get(endpoint)
if err != nil {
t.Errorf("Expected OK response. Got '%d'.", err)
} else {
resp.Body.Close()
}
}

Expand Down Expand Up @@ -723,6 +725,8 @@ func TestTwoContainersExposingTheSamePort(t *testing.T) {
if err != nil {
t.Fatal(err)
}
defer resp.Body.Close()

if resp.StatusCode != http.StatusOK {
t.Errorf("Expected status code %d. Got %d.", http.StatusOK, resp.StatusCode)
}
Expand All @@ -736,6 +740,8 @@ func TestTwoContainersExposingTheSamePort(t *testing.T) {
if err != nil {
t.Fatal(err)
}
defer resp.Body.Close()

if resp.StatusCode != http.StatusOK {
t.Errorf("Expected status code %d. Got %d.", http.StatusOK, resp.StatusCode)
}
Expand Down Expand Up @@ -766,6 +772,8 @@ func TestContainerCreation(t *testing.T) {
if err != nil {
t.Fatal(err)
}
defer resp.Body.Close()

if resp.StatusCode != http.StatusOK {
t.Errorf("Expected status code %d. Got %d.", http.StatusOK, resp.StatusCode)
}
Expand Down Expand Up @@ -893,6 +901,8 @@ func TestContainerCreationWithName(t *testing.T) {
if err != nil {
t.Fatal(err)
}
defer resp.Body.Close()

if resp.StatusCode != http.StatusOK {
t.Errorf("Expected status code %d. Got %d.", http.StatusOK, resp.StatusCode)
}
Expand Down Expand Up @@ -925,6 +935,8 @@ func TestContainerCreationAndWaitForListeningPortLongEnough(t *testing.T) {
if err != nil {
t.Fatal(err)
}
defer resp.Body.Close()

if resp.StatusCode != http.StatusOK {
t.Errorf("Expected status code %d. Got %d.", http.StatusOK, resp.StatusCode)
}
Expand Down Expand Up @@ -979,6 +991,8 @@ func TestContainerRespondsWithHttp200ForIndex(t *testing.T) {
if err != nil {
t.Error(err)
}
defer resp.Body.Close()

if resp.StatusCode != http.StatusOK {
t.Errorf("Expected status code %d. Got %d.", http.StatusOK, resp.StatusCode)
}
Expand Down Expand Up @@ -1088,6 +1102,7 @@ func Test_BuildContainerFromDockerfileWithBuildArgs(t *testing.T) {
if err != nil {
t.Fatal(err)
}
defer resp.Body.Close()

body, err := io.ReadAll(resp.Body)
if err != nil {
Expand Down Expand Up @@ -1378,6 +1393,8 @@ func TestContainerCreationWithBindAndVolume(t *testing.T) {
t.Cleanup(func() {
ctx, cnl := context.WithTimeout(context.Background(), 5*time.Second)
defer cnl()
defer dockerCli.Close()

err := dockerCli.VolumeRemove(ctx, volumeName, true)
if err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -1473,7 +1490,7 @@ func TestContainerNonExistentImage(t *testing.T) {
t.Fatalf("err should be a ctx cancelled error %v", err)
}

terminateContainerOnEnd(t, ctx, c)
terminateContainerOnEnd(t, context.Background(), c) // use non-cancelled context
})
}

Expand Down Expand Up @@ -1518,6 +1535,7 @@ func TestContainerCustomPlatformImage(t *testing.T) {

dockerCli, err := NewDockerClient()
require.NoError(t, err)
defer dockerCli.Close()

ctr, err := dockerCli.ContainerInspect(ctx, c.GetContainerID())
assert.NoError(t, err)
Expand Down Expand Up @@ -1557,6 +1575,7 @@ func readHostname(tb testing.TB, containerId string) string {
if err != nil {
tb.Fatalf("Failed to create Docker client: %v", err)
}
defer containerClient.Close()

containerDetails, err := containerClient.ContainerInspect(context.Background(), containerId)
if err != nil {
Expand Down Expand Up @@ -1668,6 +1687,7 @@ func TestDockerCreateContainerWithFiles(t *testing.T) {
},
Started: false,
})
terminateContainerOnEnd(t, ctx, nginxC)

if err != nil {
require.Contains(t, err.Error(), tc.errMsg)
Expand Down Expand Up @@ -1752,6 +1772,7 @@ func TestDockerCreateContainerWithDirs(t *testing.T) {
},
Started: false,
})
terminateContainerOnEnd(t, ctx, nginxC)

require.True(t, (err != nil) == tc.hasError)
if err == nil {
Expand Down Expand Up @@ -1940,11 +1961,15 @@ func TestContainerWithReaperNetwork(t *testing.T) {
Name: nw,
Attachable: true,
}
_, err := GenericNetwork(ctx, GenericNetworkRequest{
n, err := GenericNetwork(ctx, GenericNetworkRequest{
ProviderType: providerType,
NetworkRequest: nr,
})
assert.Nil(t, err)
defer func() {
err := n.Remove(ctx)
assert.NoError(t, err)
}()
}

req := ContainerRequest{
Expand Down Expand Up @@ -2105,6 +2130,8 @@ func TestGetGatewayIP(t *testing.T) {
if err != nil {
t.Fatal(err)
}
defer provider.Close()

ip, err := provider.(*DockerProvider).GetGatewayIP(context.Background())
if err != nil {
t.Fatal(err)
Expand Down
1 change: 1 addition & 0 deletions generic.go
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ func GenericContainer(ctx context.Context, req GenericContainerRequest) (Contain
if err != nil {
return nil, err
}
defer provider.Close()

var c Container
if req.Reuse {
Expand Down
1 change: 1 addition & 0 deletions internal/testcontainersdocker/docker_host.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ func extractDockerSocket(ctx context.Context) string {
if err != nil {
panic(err) // a Docker client is required to get the Docker info
}
defer cli.Close()

return extractDockerSocketFromClient(ctx, cli)
}
Expand Down
1 change: 1 addition & 0 deletions mounts_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,7 @@ func TestCreateContainerWithVolume(t *testing.T) {
// Check if volume is created
client, err := NewDockerClient()
assert.NoError(t, err)
defer client.Close()

volume, err := client.VolumeInspect(ctx, "test-volume")
assert.NoError(t, err)
Expand Down
2 changes: 2 additions & 0 deletions network_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,8 @@ func Test_NetworkWithIPAM(t *testing.T) {
if err != nil {
t.Fatal("Cannot get Provider")
}
defer provider.Close()

foundNetwork, err := provider.GetNetwork(ctx, NetworkRequest{Name: networkName})
if err != nil {
t.Fatal("Cannot get created network by name")
Expand Down

0 comments on commit 3febcdc

Please sign in to comment.