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.

Sign up for GitHub

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

Jump to bottom

all: fix goroutine leaks #1358

Merged
merged 1 commit into from
Aug 9, 2023
Merged

all: fix goroutine leaks #1358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
47 changes: 31 additions & 16 deletions docker.go
View file
Open in desktop
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()
mdelapenya marked this conversation as resolved.
Show resolved Hide resolved

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
View file
Open in desktop
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
View file
Open in desktop
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
mdelapenya marked this conversation as resolved.
Show resolved Hide resolved
})
}

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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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