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: avoid checking for the reaper out of its mutex #1692

Merged

Conversation

mdelapenya
Copy link
Member

@mdelapenya mdelapenya commented Sep 27, 2023

What does this PR do?

This PR removes redudant checks for getting the test session ID, simply delegating it to the sessionID global value, not reading it from the reaper (which already uses the very same value).

Why is it important?

Having a nil-check for the reaperInstance to get the test sessionID introduced a race condition, as it's only set to not nil under a code block that is protected by a Mutex. Therefore, the check caused problems instead of benefits. I added that check intentionally, thinking that always reading from the reaper could be a good idea. But it was not.

Related issues

How to test this PR

This should demonstrate the bug (before this fix) and the fix:

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

Also running all reaper tests:

go test -timeout 600s -run "^(TestContainerStartsWithoutTheReaper|TestContainerStartsWithTheReaper|TestContainerStopWithReaper|TestContainerTerminationWithReaper|TestContainerTerminationWithoutReaper|Test_NewReaper|Test_ReaperForNetwork|Test_ReaperReusedIfHealthy|TestReaper_reuseItFromOtherTestProgramUsingDocker)$" github.com/testcontainers/testcontainers-go -count=1 -race -v

@mdelapenya mdelapenya requested a review from a team as a code owner September 27, 2023 11:14
@mdelapenya mdelapenya added the bug An issue with the library label Sep 27, 2023
@mdelapenya mdelapenya self-assigned this Sep 27, 2023
@netlify
Copy link

netlify bot commented Sep 27, 2023

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit 3a14ffe
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/65140ea012db14000858ec4c
😎 Deploy Preview https://deploy-preview-1692--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 configuration.

@mdelapenya
Copy link
Member Author

As a follow up, I think we could include the -race flag whenever running tests in the CI. I think it's a good practice that could help us identify race conditions the soonest.

I think I'll do it in a separate PR.

@mdelapenya mdelapenya merged commit e7e26eb into testcontainers:main Sep 27, 2023
106 checks passed
@mdelapenya mdelapenya deleted the fix-race-condition-reaper branch September 28, 2023 14:21
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.

[Bug]: Data Race accessing reaper instance for docker containers
1 participant