all: fix goroutine leaks

[AlexanderYastrebov]

Fixes various goroutine leaks by moving around and adding missing Close calls.

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

// 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

What does this PR do?

Why is it important?

Related issues

[netlify]

✅ Deploy Preview for testcontainers-go ready!

Name	Link
🔨 Latest commit	2b055df
🔍 Latest deploy log	https://app.netlify.com/sites/testcontainers-go/deploys/64cfcc0b81026d000882479e
😎 Deploy Preview	https://deploy-preview-1358--testcontainers-go.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[AlexanderYastrebov]

Fixed golangci-lint "Error return value of n.Remove is not checked (errcheck)"

[mdelapenya]

I left a few minor comments, will postpone the final review until we discuss them.

Other than that, LGTM. Thanks for your dedication!

[AlexanderYastrebov]

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

There is another tricky case besides #1357 - reused containers.
In the TestParallelContainersWithReuse reused container has reaper that would not be closed:

testcontainers-go/parallel_test.go

Lines 163 to 169 in 5011016

	res, err := ParallelContainers(ctx, parallelRequest, ParallelContainersOptions{})
	if err != nil {
	e, _ := err.(ParallelContainersError)
	t.Fatalf("expected errors: %d, got: %d\n", 0, len(e.Errors))
	}
	// Container is reused, only terminate first container
	terminateContainerOnEnd(t, ctx, res[0])

Closing all containers produces error as there is only one real container.

In the TestGenericReusableContainer reused container also can not be closed but even if we remove reaper it still calls Exec:

testcontainers-go/generic_test.go

Lines 90 to 94 in 5011016

	if err == nil {
	c, _, err := n2.Exec(ctx, []string{"/bin/ash", copiedFileName})
	require.NoError(t, err)
	require.Zero(t, c)
	}

which creates an idle connection in the client and the problem is that reused containers have their own provider:

testcontainers-go/generic.go

Lines 125 to 138 in 5011016

	provider, err := req.ProviderType.GetProvider(WithLogger(logging))
	if err != nil {
	return nil, err
	}
	var c Container
	if req.Reuse {
	// we must protect the reusability of the container in the case it's invoked
	// in a parallel execution, via ParallelContainers or t.Parallel()
	reuseContainerMx.Lock()
	defer reuseContainerMx.Unlock()
	c, err = provider.ReuseOrCreateContainer(ctx, req.ContainerRequest)
	} else {

testcontainers-go/docker.go

Lines 1139 to 1149 in 5011016

	dc := &DockerContainer{
	ID: c.ID,
	WaitingFor: req.WaitingFor,
	Image: c.Image,
	sessionID: testcontainerssession.ID(),
	provider: p,
	terminationSignal: termSignal,
	stopProducer: nil,
	logger: p.Logger,
	isRunning: c.State == "running",
	}

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
7.4% Duplication

[mdelapenya]

LGTM! Thanks for your patience while reviewing the PR.

As you seem very proficient with goroutines, would you participate in an eventual redesign for a revamp of the current Docker client infrastructure, in a v1 branch?

[AlexanderYastrebov]

@mdelapenya Thank you.

would you participate in an eventual redesign

Just tag me and I'll think whether I could contribute anything meaningful.