fix(reaper): fix race condition when reusing reapers#1904
Merged
mdelapenya merged 4 commits intotestcontainers:mainfrom Nov 8, 2023
Merged
fix(reaper): fix race condition when reusing reapers#1904mdelapenya merged 4 commits intotestcontainers:mainfrom
mdelapenya merged 4 commits intotestcontainers:mainfrom
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Reaper reuse/creation logic has been adjusted to facilitate the reuse of already running reapers. This includes using a specific naming convention based on the session id, and handling failed container creation attempts due to name conflicts by retrieving the already running reaper container. This fixes a race condition when tests are run in parallel in multiple packages which renders global locks ineffective.
lefinal
force-pushed
the
fix-ryuk-race-condition
branch
from
4bb3a7b to
37d3093
Compare
lefinal
changed the title
mdelapenya
reviewed
Nov 6, 2023
mdelapenya
reviewed
Nov 6, 2023
mdelapenya
previously approved these changes
Nov 6, 2023
Member
mdelapenya
left a comment
mdelapenya
left a comment
There was a problem hiding this comment.
Hey @lefinal thanks for this fix. You're doing a great job investigating the potential race conditions in the project, which I enormously appreciate it.
I took the test code and put it into the main branch and it failed, so it demonstrated the bug. Same test in this branch does pass 👏
In any case, there a few comments to be addressed, but I'm approving this PR already. LGTM
The log message related to the reaper container in the reaper.go file has been updated for better clarity. The redundant phrase "Canceling creation -" has been removed as it does not provide additional relevant information, aiming to improve log readability.
Further comments where added to error handling in the reaper.go file, to better account for possible race conditions. This includes conditions where a container creation fails due to name conflict but no containers are visible in list-requests. Additionally, a possible scenario where the container may have died between requests has been covered.
lefinal
force-pushed
the
fix-ryuk-race-condition
branch
from
ef6e2ae to
a006d8d
Compare
lefinal
force-pushed
the
fix-ryuk-race-condition
branch
from
a006d8d to
181e7bc
Compare
mdelapenya
approved these changes
Nov 8, 2023
Member
mdelapenya
left a comment
mdelapenya
left a comment
There was a problem hiding this comment.
LGTM, thanks for your patience during the review 🙇
mdelapenya
added a commit
to mdelapenya/testcontainers-go
that referenced
this pull request
Nov 8, 2023
* main: fix(reaper): fix race condition when reusing reapers (testcontainers#1904)
mdelapenya
added a commit
to kuisathaverat/testcontainers-go
that referenced
this pull request
Nov 20, 2023
* main: (31 commits) feat: support for executing commands in a container with user, workDir and env (testcontainers#1914) fix(modules.kafka): Switch to MaxInt for 32-bit support (testcontainers#1923) docs: fix code snippet for image substitution (testcontainers#1918) Add database driver note to SQL Wait strategy docs (testcontainers#1916) Reduce flakiness in ClickHouse tests (testcontainers#1902) lint: enable nonamedreturns (testcontainers#1909) chore: deprecate BindMount APIs (testcontainers#1907) fix(reaper): fix race condition when reusing reapers (testcontainers#1904) feat: Allow the container working directory to be specified (testcontainers#1899) chore: make rabbitmq examples more readable (testcontainers#1905) chore(deps): bump github.com/twmb/franz-go and github.com/twmb/franz-go/pkg/kadm in /modules/redpanda (testcontainers#1896) Fix - respect ContainerCustomizer in neo4j module (testcontainers#1903) chore(deps): bump github.com/nats-io/nkeys and github.com/nats-io/nats.go in /modules/nats (testcontainers#1897) chore: add tests for withNetwork option (testcontainers#1894) chore(deps): bump google.golang.org/grpc and cloud.google.com/go/firestore in /modules/gcloud (testcontainers#1891) chore(deps): bump github.com/aws/aws-sdk-go and github.com/aws/aws-sdk-go-v2/config in /modules/localstack (testcontainers#1892) chore(deps): bump Github actions (testcontainers#1890) chore(deps): bump github.com/shirou/gopsutil/v3 from 3.23.9 to 3.23.10 (testcontainers#1858) chore(deps): bump github.com/hashicorp/consul/api in /examples/consul (testcontainers#1863) chore(deps): bump github.com/IBM/sarama in /modules/kafka (testcontainers#1874) ...
mdelapenya
added a commit
to mdelapenya/testcontainers-go
that referenced
this pull request
Nov 30, 2023
* main: (100 commits) fix: fallback matching of registry authentication config (testcontainers#1927) feat: support customizing the Docker build command (testcontainers#1931) docs: include MongoDB's username and password options into the docs (testcontainers#1930) feat: support for custom registry prefixes at the configuration level (testcontainers#1928) Add username and password functions to mongodb (testcontainers#1910) chore: skip TestContainerLogWithErrClosed as flaky on rootless docker (testcontainers#1925) docs: add some Vault module examples (testcontainers#1825) feat: support for executing commands in a container with user, workDir and env (testcontainers#1914) fix(modules.kafka): Switch to MaxInt for 32-bit support (testcontainers#1923) docs: fix code snippet for image substitution (testcontainers#1918) Add database driver note to SQL Wait strategy docs (testcontainers#1916) Reduce flakiness in ClickHouse tests (testcontainers#1902) lint: enable nonamedreturns (testcontainers#1909) chore: deprecate BindMount APIs (testcontainers#1907) fix(reaper): fix race condition when reusing reapers (testcontainers#1904) feat: Allow the container working directory to be specified (testcontainers#1899) chore: make rabbitmq examples more readable (testcontainers#1905) chore(deps): bump github.com/twmb/franz-go and github.com/twmb/franz-go/pkg/kadm in /modules/redpanda (testcontainers#1896) Fix - respect ContainerCustomizer in neo4j module (testcontainers#1903) chore(deps): bump github.com/nats-io/nkeys and github.com/nats-io/nats.go in /modules/nats (testcontainers#1897) ...
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reaper reuse/creation logic has been adjusted to facilitate the reuse of already running reapers. This includes using a specific naming convention based on the session id, and handling failed container creation attempts due to name conflicts by retrieving the already running reaper container. This fixes a race condition when tests are run in parallel in multiple packages which renders global locks ineffective.
What does this PR do?
Reaper container now uses a deterministic name based on the session id.
If container creation fails due to name conflict, we reuse the existing container.
Why is it important?
If an application uses testcontainers in multiple packages,
go testwill run them isolated from each other.This will render global locks as in reaper.go ineffective causing a race condition as multiple reapers are created.
However, they all use the same session id, because of the parent process being the same.
This may lead to unwanted cancellation of containers of still running tests in other packages.
There is already logic for reducing the risk of race conditions in reaper.go by introducing a small random delay when searching for any running reaper container.
However, this does not fully avoid the race condition as there is a small delay after container creation and it being visible from
ContainerList.How to test this PR
Tests have been added to reaper_test.go.
You can test the functionality by simply discarding the changes in reaper.go and running the tests.