fix: support parallel execution of reusable containers

[mdelapenya]

What does this PR do?

It adds a sync.Mutex lock/unlock when a reusable container is created, including a test that demonstrates the bug

In the original bug report (#592), a repro snippet was added, although I preferred a different test that leverages our testcontainers.ParallelContainers API, adding code coverage to the library.

Why is it important?

Thanks to @mhatch-nxcr, who reported this bug in #592, we demonstrated that the implementation of the reusable containers was not complete in terms of a parallel execution of tests creating containers with the library. The bug report found a race condition while checking the Docker client to find containers with the same name when called from a goroutine (i.e. running multiple Test functions including t.Parallel() or our ParallelContainers function).

Internally, when we are checking if we need to reuse a container by its name, the library calls the Docker client with a findContainerByName function of the DockerProvider. At the moment this findContainerByName function was called by any goroutine, no container was already created with the reusable container name, and in consequence the Docker client responded with a list of zero containers, which was expected. Therefore we had to look up the stack call to identify what code was calling that method, until we found that we needed synchronization at the moment the first container was created. That's why we are adding the lock/unlock only for the req.Reuse if/else branch.

Before the changes

graph TD
    B(Test execution)
    B --> C{Parallel creation}
    C -->|ReuseContainer| D[Name: reusable]
    C -->|ReuseContainer| E[Name: reusable]
    C -->|ReuseContainer| F[Name: reusable]
    D -->|findContainerByName| G[Docker client]
    E -->|findContainerByName| G[Docker client]
    F -->|findContainerByName| G[Docker client]
    G -->|filter:name=reusable| H["len(List)"]

After the changes

graph TD
    B(Test execution)
    B --> C{Parallel creation}
    C --> I[sync.Mutex]
    I -->|ReuseContainer| D[Name: reusable]
    I -->|ReuseContainer| E[Name: reusable]
    I -->|ReuseContainer| F[Name: reusable]
    D -->|findContainerByName| G[Docker client]
    E -->|findContainerByName| G[Docker client]
    F -->|findContainerByName| G[Docker client]
    G -->|filter:name=reusable| H["len(List)"]

How to test this

Run the first commit of this PR with and without the second commit.

Another way to test this fix is using what @mhatch-nxcr proposed in the original bug report: because the two tests uses t.Parallel(), they will be run in different goroutines, being affected by the bug. Please run all tests in the reusable package, not only one of them.

package reusable
​
import (
	"context"
	"fmt"
	"testing"
	"time"
​
	"github.com/docker/go-connections/nat"
	testcontainers "github.com/testcontainers/testcontainers-go"
	"github.com/testcontainers/testcontainers-go/wait"
)
​
const (
	postgresPort     = 5432
	postgresPassword = "test"
	postgresUser     = "test"
	postgresDb       = "test"
)
​
func TestSomething(t *testing.T) {
	t.Parallel()
​
	_, _ = mustSetupPostgresDB(t)
}
​
func TestSomething2(t *testing.T) {
	t.Parallel()
​
	_, _ = mustSetupPostgresDB(t)
}
​
func mustSetupPostgresDB(t *testing.T) (string, testcontainers.Container) {
	natPort := fmt.Sprintf("%d/tcp", postgresPort)
	pg, err := testcontainers.GenericContainer(
		context.Background(),
		testcontainers.GenericContainerRequest{
			ContainerRequest: testcontainers.ContainerRequest{
				Image:        "postgis/postgis",
				Name:         "test-postgres",
				ExposedPorts: []string{natPort},
				Env: map[string]string{
					"POSTGRES_PASSWORD": postgresPassword,
					"POSTGRES_USER":     postgresUser,
					"POSTGRES_DATABASE": postgresDb,
				},
				WaitingFor: wait.ForLog("database system is ready to accept connections").
					WithPollInterval(100 * time.Millisecond).
					WithOccurrence(2),
			},
			Started: true,
			Reuse:   true,
		},
	)
	if err != nil {
		t.Fatal(err)
	}
​
	// Even after log message found Postgres needs a touch more...
	time.Sleep(200 * time.Millisecond)
​
	// Get the container info needed
	port, err := pg.MappedPort(context.Background(), nat.Port(natPort))
	if err != nil {
		t.Fatal(err)
	}
​
	host, err := pg.Host(context.Background())
	if err != nil {
		t.Fatal(err)
	}
​
	return fmt.Sprintf(
		"postgresql://%s:%s@%s:%d/%s?sslmode=disable",
		postgresUser,
		postgresPassword,
		host,
		port.Int(),
		postgresDb,
	), pg
}

Related issues

• Fixes [Bug]: Container reuse fails with "The container name X is already in use by container" #592

[kiview]

Going ahead, we already know that the approach of reusing by name is not ideal and we want to change it. I assume this bug, and accordingly the fix, is independent of the mechanism of how the lookup works, correct?

[mdelapenya]

Going ahead, we already know that the approach of reusing by name is not ideal and we want to change it. I assume this bug, and accordingly the fix, is independent of the mechanism of how the lookup works, correct?

Exactly, and we have an issue to refactor the lookup mechanism here: #568

[mhatch-nxcr]

simple, effective. 👍