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.

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

Fix race condition while initializing docker client #1160

Merged
merged 1 commit into from May 8, 2023

Conversation

nhatthm
Copy link
Contributor

@nhatthm nhatthm commented May 8, 2023

What does this PR do?

After updating the dependency in my project from v0.19.0 to v0.20.0, I get this error while running the tests

==================
WARNING: DATA RACE
Read at 0x00010366fa60 by goroutine 27:
  github.com/testcontainers/testcontainers-go.ReadConfig()
      /go/src/github.com/nhatthm/testcontainers-go-extra/vendor/github.com/testcontainers/testcontainers-go/config.go:45 +0x5c
  github.com/testcontainers/testcontainers-go.NewDockerClient()
      /go/src/github.com/nhatthm/testcontainers-go-extra/vendor/github.com/testcontainers/testcontainers-go/docker.go:787 +0x30
  github.com/testcontainers/testcontainers-go.NewDockerProvider()
      /go/src/github.com/nhatthm/testcontainers-go-extra/vendor/github.com/testcontainers/testcontainers-go/provider.go:139 +0x1e4
  github.com/testcontainers/testcontainers-go.ProviderType.GetProvider()
      /go/src/github.com/nhatthm/testcontainers-go-extra/vendor/github.com/testcontainers/testcontainers-go/provider.go:111 +0x55c
  github.com/testcontainers/testcontainers-go.GenericContainer()
      /go/src/github.com/nhatthm/testcontainers-go-extra/vendor/github.com/testcontainers/testcontainers-go/generic.go:125 +0x110
  go.nhat.io/testcontainers-extra.StartGenericContainer()
      /go/src/github.com/nhatthm/testcontainers-go-extra/container.go:47 +0x2a8
  go.nhat.io/testcontainers-extra.StartGenericContainers.func1()
      /go/src/github.com/nhatthm/testcontainers-go-extra/container.go:83 +0xcc
  go.nhat.io/testcontainers-extra.StartGenericContainers.func2()
      /go/src/github.com/nhatthm/testcontainers-go-extra/container.go:95 +0xa4

Previous write at 0x00010366fa60 by goroutine 15:
  github.com/testcontainers/testcontainers-go.NewDockerClient()
      /go/src/github.com/nhatthm/testcontainers-go-extra/vendor/github.com/testcontainers/testcontainers-go/docker.go:787 +0x78
  github.com/testcontainers/testcontainers-go.NewDockerProvider()
      /go/src/github.com/nhatthm/testcontainers-go-extra/vendor/github.com/testcontainers/testcontainers-go/provider.go:139 +0x1e4
  github.com/testcontainers/testcontainers-go.ProviderType.GetProvider()
      /go/src/github.com/nhatthm/testcontainers-go-extra/vendor/github.com/testcontainers/testcontainers-go/provider.go:111 +0x55c
  github.com/testcontainers/testcontainers-go.GenericContainer()
      /go/src/github.com/nhatthm/testcontainers-go-extra/vendor/github.com/testcontainers/testcontainers-go/generic.go:125 +0x110
  go.nhat.io/testcontainers-extra.StartGenericContainer()
      /go/src/github.com/nhatthm/testcontainers-go-extra/container.go:47 +0x2a8
  go.nhat.io/testcontainers-extra_test.TestStartGenericContainer_NamePrefixAndSuffix()
      /go/src/github.com/nhatthm/testcontainers-go-extra/container_test.go:119 +0xf8
  testing.tRunner()
      /opt/homebrew/opt/go/libexec/src/testing/testing.go:1576 +0x188
  testing.(*T).Run.func1()
      /opt/homebrew/opt/go/libexec/src/testing/testing.go:1629 +0x40

The root cause is tcConfig = ReadConfig(), tcConfig is a private global variable and is initialized in the ReadConfig(). The one in NewDockerClient() should be a local variable instead.

Why is it important?

The testcontainers should be running without a race condition

Related issues

n/a

@nhatthm nhatthm requested a review from a team as a code owner May 8, 2023 20:01
@netlify
Copy link

netlify bot commented May 8, 2023

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit c7e3ca9
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/64595503a812bf00087c8726
😎 Deploy Preview https://deploy-preview-1160--testcontainers-go.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@sonarcloud
Copy link

sonarcloud bot commented May 8, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@mdelapenya mdelapenya self-assigned this May 8, 2023
@mdelapenya mdelapenya added the bug An issue with the library label May 8, 2023
@mdelapenya
Copy link
Collaborator

I've thinking about adding the -race flag since ever but never found time. And I think this is the perfect timing to do so.

Copy link
Collaborator

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for fixing the issue that quick!

@mdelapenya mdelapenya merged commit b4572f5 into testcontainers:main May 8, 2023
56 checks passed
@nhatthm nhatthm deleted the fix-race-condition branch May 8, 2023 20:59
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request May 11, 2023
mdelapenya added a commit that referenced this pull request May 26, 2023
* chore: add helper function to check if a file exists

* chore: wrap the extraction of the current docker host into a reusable function

* chore: extract part of the algorithm to calculate the docker socket

* chore: use constant for default docker socket in tests

* chore: extract docker socket from Go context to a function

* chore: reuse constant for default docker socket path

* chore: detect DOCKER_HOST first

* chore: do not fallback to default, return empty string

* chore: apply extract docker host function

* chore: read properties with context

* docs: document the docker host detection strategies

* feat: look up rootless docker if docker host was not found

* chore: extract modules generator pipeline

* chore: add a pipeline for rootless docker

* chore: remove duplicated pipeline

* chore: define docekr socket path for unix/windows

* chore: convert internal constants into vars for testability

* chore: use t.Cleanup to restore test state

* feat: support for reading the default docker host from unix/windows

* chore: move config to internal package

* chore: unify how the docker client is retrieved

* chore: delegate singleton instance to the internal config

* chore: remove DOCKER_HOST env var from config

* chore: support resetting the config singleton for testing

* feat: read docker host from properties

* fix: typo

* fix: remove white lines in imports

* fix: handle error in tests

* chore: apply #1160 after merge

* chore: do not go through the provider to get a client

* fix: honour passed Docker client opts

* fix: read properties does not consider reading DOCKER_HOST env var

* fix: defer client close after docker ping

* feat: extract the docker host when bootstrapping a docker client

* chore: honour passed opts for the client

* chore: cache docker_host to avoid unnecesary calculations

* chor: convert socket schema into constants

* chore: support parsing URLs for remote hosts

* fix: reaper mounts a socket path without schema

* fix: proper comment structure for go linting using golangci-lint

* chore: get socket mount without schema

* fix: support for disabling rootless for testing

* fix: get provider host from the strategies

* chore: proper separation of concerns between docker socket and docker host

* docs: improve docs for Docker host detection

* fix: comments indentation

* fix: proper list format in Go comments

* docs: document rootless socket path alternatives in code

* feat: support for testcontainers host strategy

* docs: include new strategy in go comment

* fix: use tc.host

* fix: proper initialisation of baseRunDir

* fix: unset DOCKER_HOST for tests that does not need it

* chore: do not print config

* fix: include schema in the rootless docker socket path

* fix: add schema properly

* fix: always prepend the schema

$XDG_RUNTIME_DIR does not contain it

* fix: proper return when sanitising the docker socket path

* chore: proper use of docker socket path

* chore: defensively restore env var in tests

* fix: make sure the already built image is removed from the local cache

* fix: use testcontainers docker client when removing image

* fix: update docs

* chore: rename variable

* fix: add back module generator tests

* chore: do not break users for the deprecated fields in the tc config

* fix: tc.host takes precedence on TESTCONTAINERS_DOCKER_SOCKET_OVERRIDE for the docker socket
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue with the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants