Skip to content

Commit

Permalink
all: fix goroutine leaks (#1358)
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 Aug 9, 2023
1 parent 0766ef4 commit 7b4cf84
Show file tree
Hide file tree
Showing 6 changed files with 74 additions and 21 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 @@ -141,6 +141,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
43 changes: 38 additions & 5 deletions docker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -200,9 +200,14 @@ 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)
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 @@ -723,6 +728,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 +743,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 +775,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 +904,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 +938,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 @@ -977,8 +992,10 @@ func TestContainerRespondsWithHttp200ForIndex(t *testing.T) {
}
resp, err := http.Get(origin)
if err != nil {
t.Error(err)
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 @@ -1088,6 +1105,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 +1396,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 +1493,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 +1538,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 +1578,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 +1690,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 +1775,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 @@ -1829,6 +1853,7 @@ func TestDockerContainerCopyFileFromContainer(t *testing.T) {
if err != nil {
t.Fatal(err)
}
defer reader.Close()

fileContentFromContainer, err := io.ReadAll(reader)
if err != nil {
Expand Down Expand Up @@ -1867,6 +1892,7 @@ func TestDockerContainerCopyEmptyFileFromContainer(t *testing.T) {
if err != nil {
t.Fatal(err)
}
defer reader.Close()

fileContentFromContainer, err := io.ReadAll(reader)
if err != nil {
Expand Down Expand Up @@ -1940,11 +1966,16 @@ 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)
// use t.Cleanup to run after terminateContainerOnEnd
t.Cleanup(func() {
err := n.Remove(ctx)
assert.NoError(t, err)
})
}

req := ContainerRequest{
Expand Down Expand Up @@ -2105,6 +2136,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 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 7b4cf84

Please sign in to comment.