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

[Bug]: Data Race accessing reaper instance for docker containers #1691

Closed
davidsbond opened this issue Sep 26, 2023 · 4 comments · Fixed by #1692
Closed

[Bug]: Data Race accessing reaper instance for docker containers #1691

davidsbond opened this issue Sep 26, 2023 · 4 comments · Fixed by #1692
Assignees
Labels
bug An issue with the library

Comments

@davidsbond
Copy link

Testcontainers version

0.24.1

Using the latest Testcontainers version?

Yes

Host OS

Linux

Host arch

amd64

Go version

1.21

Docker version

24.0.6

Docker info

This is running in github actions so I don't have access to it.

What happened?

When using testcontainers with the -race flag enabled, a race is detected within the testcontainers package. It seems to be due to some code paths not using the reaperMux when accessing the session identifiers. For example here:

https://github.com/testcontainers/testcontainers-go/blob/main/docker.go#L881

Relevant log output

WARNING: DATA RACE
Write at 0x000001a3cc50 by goroutine 17:
  github.com/testcontainers/testcontainers-go.reuseOrCreateReaper()
      /home/runner/go/pkg/mod/github.com/testcontainers/[email protected]/reaper.go:155 +0x624

Previous read at 0x000001a3cc50 by goroutine 14:
  github.com/testcontainers/testcontainers-go.(*DockerProvider).CreateContainer()
      /home/runner/go/pkg/mod/github.com/testcontainers/[email protected]/docker.go:881 +0xb34

Additional information

No response

@davidsbond davidsbond added the bug An issue with the library label Sep 26, 2023
@mdelapenya mdelapenya self-assigned this Sep 26, 2023
@mdelapenya
Copy link
Member

Oh, you are totally right! I introduced the race condition checking if the reaperInstance is nil 🤦

I'll work on a fix ASAP. Thanks for reporting this.

Just to let you know, we are working on a draft document for an eventual v1 release, in which we'd like to refine all the bootstrapping of the library, in particular how config, reaper and test session is initialised. So it would be fixed at that time in a more proper manner. Nevertheless, I'll tackle this bug now.

@mdelapenya
Copy link
Member

I can reproduce the bug with:

go test -timeout 600s -run ^Test_BuildImageWithContexts$ . -race -count=1 -v

I also have a working branch without that error.

@mdelapenya
Copy link
Member

A PR fix has been sent, thanks for the report!

@davidsbond
Copy link
Author

Thanks for the quick look! Glad to help :)

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 a pull request may close this issue.

2 participants