fix: avoid double lock in DockerProvider.DaemonHost()#2900
fix: avoid double lock in DockerProvider.DaemonHost()#2900mdelapenya merged 11 commits intotestcontainers:mainfrom
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
stevenh
left a comment
stevenh
left a comment
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. |
stevenh
left a comment
stevenh
left a comment
There was a problem hiding this comment.
Thanks for the rework, have suggested a few changes and a question.
stevenh
left a comment
stevenh
left a comment
There was a problem hiding this comment.
Unfortunately the test is failing in rootless setup, see CI output.
|
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. |
mdelapenya
left a comment
mdelapenya
left a comment
There was a problem hiding this comment.
LGTM, thank you both for your time here 🙇
* main: (103 commits) feat(postgres): ssl for postgres (testcontainers#2473) feat(ollama): support calling the Ollama local process (testcontainers#2923) chore(deps): bump jinja2 from 3.1.4 to 3.1.5 (testcontainers#2935) chore(deps): bump sonarsource/sonarcloud-github-action (testcontainers#2933) feat(termination)!: make container termination timeout configurable (testcontainers#2926) chore(deps): bump slackapi/slack-github-action from 1.26.0 to 2.0.0 (testcontainers#2934) chore(deps): bump github/codeql-action from 3.25.15 to 3.28.0 (testcontainers#2932) feat(wait): log sub match callback (testcontainers#2929) fix: Handle nil value in CleanupNetwork (testcontainers#2928) fix: avoid double lock in DockerProvider.DaemonHost() (testcontainers#2900) feat!: build log writer for container request (testcontainers#2925) feat(gcloud)!: add support to seed data when using RunBigQueryContainer (testcontainers#2523) security(deps): bump golang.org/x/crypto from 0.28.0 to 0.31.0 (testcontainers#2916) chore(ci): add Github labels based on PR title (testcontainers#2914) chore(gha): Use official setup-docker-action (testcontainers#2913) chore(ci): enforce conventional commits syntax in PR titles (testcontainers#2911) feat(nats): WithConfigFile - pass a configuration file to nats server (testcontainers#2905) chore: enable implicit default logger only in testing with -v (testcontainers#2877) fix: container binds syntax (testcontainers#2899) refactor(cockroachdb): to use request driven options (testcontainers#2883) ...
3 participants
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?