fix: data race in docker client `Info()` #1779

pmalek · 2023-10-16T11:39:46Z

What does this PR do?

This fixes a data race in docker client Info() which occurs when e.g. docker socket is unreachable:

==================
WARNING: DATA RACE
Write at 0x0001098d6700 by goroutine 29:
  github.com/testcontainers/testcontainers-go.(*DockerClient).Info()
      /Users/USER/code_/testcontainers-go/docker_client.go:69 +0xa4
  github.com/testcontainers/testcontainers-go.NewDockerClientWithOpts()
      /Users/USER/code_/testcontainers-go/docker_client.go:110 +0xd4
  github.com/testcontainers/testcontainers-go.NewDockerProvider()
      /Users/USER/code_/testcontainers-go/provider.go:142 +0x1e4
  github.com/testcontainers/testcontainers-go.ProviderType.GetProvider()
      /Users/USER/code_/testcontainers-go/provider.go:113 +0x654
  github.com/testcontainers/testcontainers-go.GenericContainer()
      /Users/USER/code_/testcontainers-go/generic.go:154 +0x110
  github.com/kong/kubernetes-ingress-controller/v2/test/kongintegration/containers.NewKong()
      /Users/USER/code_/kubernetes-ingress-controller/test/kongintegration/containers/kong.go:60 +0x690
  github.com/kong/kubernetes-ingress-controller/v2/test/kongintegration.TestUpdateStrategyInMemory_PropagatesResourcesErrors()
      /Users/USER/code_/kubernetes-ingress-controller/test/kongintegration/inmemory_update_strategy_test.go:34 +0x54
  testing.tRunner()
      /Users/USER/.gvm/gos/go1.21.3/src/testing/testing.go:1595 +0x194
  testing.(*T).Run.func1()
      /Users/USER/.gvm/gos/go1.21.3/src/testing/testing.go:1648 +0x40

Previous read at 0x0001098d6700 by goroutine 28:
  ??()
      -:0 +0x106d43e30
  sync/atomic.LoadUint32()
      <autogenerated>:1 +0x10
  github.com/testcontainers/testcontainers-go.(*DockerClient).Info()
      /Users/USER/code_/testcontainers-go/docker_client.go:40 +0x8c
  github.com/testcontainers/testcontainers-go.NewDockerClientWithOpts()
      /Users/USER/code_/testcontainers-go/docker_client.go:110 +0xd4
  github.com/testcontainers/testcontainers-go.NewDockerProvider()
      /Users/USER/code_/testcontainers-go/provider.go:142 +0x1e4
  github.com/testcontainers/testcontainers-go.ProviderType.GetProvider()
      /Users/USER/code_/testcontainers-go/provider.go:113 +0x654
  github.com/testcontainers/testcontainers-go.GenericContainer()
      /Users/USER/code_/testcontainers-go/generic.go:154 +0x110
  github.com/kong/kubernetes-ingress-controller/v2/test/kongintegration/containers.NewKong()
      /Users/USER/code_/kubernetes-ingress-controller/test/kongintegration/containers/kong.go:60 +0x690
  github.com/kong/kubernetes-ingress-controller/v2/test/kongintegration.TestExpressionsRouterMatchers_GenerateValidExpressions()
      /Users/USER/code_/kubernetes-ingress-controller/test/kongintegration/expression_router_test.go:34 +0x54
  testing.tRunner()
      /Users/USER/.gvm/gos/go1.21.3/src/testing/testing.go:1595 +0x194
  testing.(*T).Run.func1()
      /Users/USER/.gvm/gos/go1.21.3/src/testing/testing.go:1648 +0x40

Goroutine 29 (running) created at:
  testing.(*T).Run()
      /Users/USER/.gvm/gos/go1.21.3/src/testing/testing.go:1648 +0x5d8
  testing.runTests.func1()
      /Users/USER/.gvm/gos/go1.21.3/src/testing/testing.go:2054 +0x80
  testing.tRunner()
      /Users/USER/.gvm/gos/go1.21.3/src/testing/testing.go:1595 +0x194
  testing.runTests()
      /Users/USER/.gvm/gos/go1.21.3/src/testing/testing.go:2052 +0x6d8
  testing.(*M).Run()
      /Users/USER/.gvm/gos/go1.21.3/src/testing/testing.go:1925 +0x908
  main.main()
      _testmain.go:85 +0x2b4

Goroutine 28 (running) created at:
  testing.(*T).Run()
      /Users/USER/.gvm/gos/go1.21.3/src/testing/testing.go:1648 +0x5d8
  testing.runTests.func1()
      /Users/USER/.gvm/gos/go1.21.3/src/testing/testing.go:2054 +0x80
  testing.tRunner()
      /Users/USER/.gvm/gos/go1.21.3/src/testing/testing.go:1595 +0x194
  testing.runTests()
      /Users/USER/.gvm/gos/go1.21.3/src/testing/testing.go:2052 +0x6d8
  testing.(*M).Run()
      /Users/USER/.gvm/gos/go1.21.3/src/testing/testing.go:1925 +0x908
  main.main()
      _testmain.go:85 +0x2b4
==================

Why is it important?

It's nice to not have data races in code.

Related issues

How to test this PR

go test -race -run TestGetDockerInfo ./...

mdelapenya

Thanks @pmalek for your contribution. I was not expecting the data race, but I could have screwed it up with the sync.Once. In any case, this PR is changing the expected behaviour of our Info method, as we want to cache it for every test session, and by test session we mean a "go test" execution.

If the data race is there, no doubt about it, please try to keep the Info cached for the entire test session, not for each docker client instance.

Thanks in advance, and please let me know if I can explain anything else with more details 🙏

docker_client.go

netlify · 2023-10-16T15:39:57Z

✅ Deploy Preview for testcontainers-go ready!

Name	Link
🔨 Latest commit	`60ba8f2`
🔍 Latest deploy log	https://app.netlify.com/sites/testcontainers-go/deploys/652e561fe149f40008b70964
😎 Deploy Preview	https://deploy-preview-1779--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.

docker_client_test.go

mdelapenya · 2023-10-17T08:49:26Z

BTW from the above log I imagine you are starting a Kong container, right? Do you know @gAmUssA created a Kong module? https://github.com/gAmUssA/testcontainers-go-kong/ You could give it a try 🤔

pmalek · 2023-10-17T09:38:22Z

BTW from the above log I imagine you are starting a Kong container, right? Do you know @gAmUssA created a Kong module? https://github.com/gAmUssA/testcontainers-go-kong/ You could give it a try 🤔

Yes, we are starting kong.

This module is indeed a more fully fledged version of what we're using and we are considering merging the efforts into one.

Thanks for the pointer though!

mdelapenya

LGTM, thanks for your time debugging race conditions. They are always hard to find and hard to fix. 🙇

* main: fix: data race in docker client `Info()` (testcontainers#1779) Use correct formatting directive for errors in lifecycle logs (testcontainers#1780) chore(deps): bump golang.org/x/mod from 0.12.0 to 0.13.0 in /modules/{elasticsearch,kafka} and /modulegen (testcontainers#1778) chore(deps): bump github.com/rabbitmq/amqp091-go in /modules/rabbitmq (testcontainers#1728) chore(deps): bump github.com/ClickHouse/clickhouse-go/v2 (testcontainers#1732) ignore patterns defined in dockerignore (testcontainers#1725)

* main: (27 commits) docs: remove OpenSSF scorecard (#1823) Auto-cleanup of k6 build cache (#1788) Add OpenSSF Scorecards GitHub Action (#1795) chore(deps): bump google.golang.org/grpc from 1.57.0 to 1.57.1 (#1822) chore: expose SessionID (#1793) chore: use HTTP calls to invoke the lambda from the tests (#1794) wait for log producer to really stop inside StopLogProducer func (#1701) chore(deps): bump github.com/nats-io/nats-server/v2 in /modules/nats (#1784) chore(deps): bump urllib3 from 2.0.6 to 2.0.7 (#1781) chore: add an example of using localstack alongside AWS lambdas (#1790) chore(deps): combine and bump compose dependencies (#1787) feat: support for replacing images with custom substitutions (#1719) fix: data race in docker client `Info()` (#1779) Use correct formatting directive for errors in lifecycle logs (#1780) chore(deps): bump golang.org/x/mod from 0.12.0 to 0.13.0 in /modules/{elasticsearch,kafka} and /modulegen (#1778) chore(deps): bump github.com/rabbitmq/amqp091-go in /modules/rabbitmq (#1728) chore(deps): bump github.com/ClickHouse/clickhouse-go/v2 (#1732) ignore patterns defined in dockerignore (#1725) Fix wrong module names (#1776) docs: add default options to k6 module (#1744) ...

…ers/image-spec-1.1.0-rc5 * main: (49 commits) docs: remove OpenSSF scorecard (#1823) Auto-cleanup of k6 build cache (#1788) Add OpenSSF Scorecards GitHub Action (#1795) chore(deps): bump google.golang.org/grpc from 1.57.0 to 1.57.1 (#1822) chore: expose SessionID (#1793) chore: use HTTP calls to invoke the lambda from the tests (#1794) wait for log producer to really stop inside StopLogProducer func (#1701) chore(deps): bump github.com/nats-io/nats-server/v2 in /modules/nats (#1784) chore(deps): bump urllib3 from 2.0.6 to 2.0.7 (#1781) chore: add an example of using localstack alongside AWS lambdas (#1790) chore(deps): combine and bump compose dependencies (#1787) feat: support for replacing images with custom substitutions (#1719) fix: data race in docker client `Info()` (#1779) Use correct formatting directive for errors in lifecycle logs (#1780) chore(deps): bump golang.org/x/mod from 0.12.0 to 0.13.0 in /modules/{elasticsearch,kafka} and /modulegen (#1778) chore(deps): bump github.com/rabbitmq/amqp091-go in /modules/rabbitmq (#1728) chore(deps): bump github.com/ClickHouse/clickhouse-go/v2 (#1732) ignore patterns defined in dockerignore (#1725) Fix wrong module names (#1776) docs: add default options to k6 module (#1744) ...

fix: data race in docker client Info()

24f01cd

pmalek requested a review from a team as a code owner October 16, 2023 11:39

programmer04 approved these changes Oct 16, 2023

View reviewed changes

mdelapenya requested changes Oct 16, 2023

View reviewed changes

docker_client.go Show resolved Hide resolved

make shared docker info a package variable

012886d

pmalek requested a review from mdelapenya October 16, 2023 15:40

mdelapenya reviewed Oct 17, 2023

View reviewed changes

docker_client_test.go Show resolved Hide resolved

Merge branch 'main' into fix-docker-client-info-data-race

60ba8f2

pmalek requested a review from mdelapenya October 17, 2023 09:41

mdelapenya self-assigned this Oct 17, 2023

mdelapenya added the bug An issue with the library label Oct 17, 2023

mdelapenya approved these changes Oct 17, 2023

View reviewed changes

mdelapenya merged commit 4ec7fa4 into testcontainers:main Oct 17, 2023
109 checks passed

pmalek deleted the fix-docker-client-info-data-race branch October 17, 2023 11:53

mdelapenya added the hacktoberfest Pull Requests accepted for Hacktoberfest. label Oct 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: data race in docker client `Info()` #1779

fix: data race in docker client `Info()` #1779

pmalek commented Oct 16, 2023

mdelapenya left a comment •

edited

Loading

netlify bot commented Oct 16, 2023 •

edited

Loading

mdelapenya commented Oct 17, 2023

pmalek commented Oct 17, 2023

mdelapenya left a comment

fix: data race in docker client Info() #1779

fix: data race in docker client Info() #1779

Conversation

pmalek commented Oct 16, 2023

What does this PR do?

Why is it important?

Related issues

How to test this PR

mdelapenya left a comment • edited Loading

Choose a reason for hiding this comment

netlify bot commented Oct 16, 2023 • edited Loading

✅ Deploy Preview for testcontainers-go ready!

mdelapenya commented Oct 17, 2023

pmalek commented Oct 17, 2023

mdelapenya left a comment

Choose a reason for hiding this comment

fix: data race in docker client `Info()` #1779

fix: data race in docker client `Info()` #1779

mdelapenya left a comment •

edited

Loading

netlify bot commented Oct 16, 2023 •

edited

Loading