fix: avoid double lock in DockerProvider.DaemonHost() #2900
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Thanks for the PR @vikstrous nice catch.
Could I ask you to take a different approach, splitting ensureDefaultNetwork into two ensureDefaultNetwork and ensureDefaultNetworkLocked avoiding the need to pass a boolean and making it clear the intent of the calls to the locked method.
Could you also construct a test which exercises this issue to ensure we don't have regression.
|
@stevenh are there existing tests where testcontainers code is being run in containers (testcontainers?)? |
|
I added a regression test. Without my change, the test fails and shows the stack trace you'd expect. With my change, it passes. The test runs in about 20s because it has to re-download go mod dependencies within the test container. This test can obviously be written a lot better, but I'd prefer it if you guys take over from here and make it conform to your best practices for writing tests. |
docker_test.go
Outdated
|
|
||
| outputBytes, err := io.ReadAll(outputReader) | ||
| require.NoError(t, err, "failed to read output") | ||
| t.Logf("%s", outputBytes) |
There was a problem hiding this comment.
question: looks like this was just debug?
There was a problem hiding this comment.
Without this, you can't see the output of stage 2, so you don't really know if it's working. I'll remove the extra print in case of error though.
There was a problem hiding this comment.
Ahh, got you because it's basically a sub test
|
I got the same error locally when I didn't bind mount /var/run. I shared the docker socket with stage 2 by bind mounting /var/run from stage 1. I don't know how this works in your CI environment / with rootless docker. I'm basically just trying to make sure that stage 2 thinks it's in a container. I'll need help with this part. |
|
I think the rootless test is actually finding another version of this bug |
|
Actually, I don't really know what's going on. I've never used rootless docker before and I think it would be best for someone to take over this PR. The bug makes testcontainers unusable within containers, which is a common scenario, so I want to get it fixed soon. |
|
Just a quick note to say this on my list thank @vikstrous just need to find some time, appreciate your time on this 😄 |
Fix the DaemonHost locking test by implementing a way to change the location of the file the core library tests for.
|
Nice improvement! Much simpler than actually running in a container. |
What does this PR do?
I carved out a code path for the DockerProvider.DaemonHost() code that avoids double locking.
Why is it important?
I was getting a deadlock. See #2897
Related issues
How to test this PR
See #2897
Follow-ups
Is there a test for running testcontainers from within a container?