Repository navigation
fix(core): create Ryuk once when containers start concurrently - #1146
Open
akshit-khandelwal47 wants to merge 1 commit into
Open
akshit-khandelwal47 wants to merge 1 commit into
akshit-khandelwal47 wants to merge 1 commit into
Conversation
Reaper.get_instance() checked and set the class-level instance without a lock, so two threads starting containers at the same time could both see no instance and both try to create testcontainers-ryuk-<session>. The second create failed with a 409 Conflict. Guard creation with a double-checked RLock. It is reentrant so that a nested get_instance() on the same thread, while Ryuk itself is starting, behaves as it did before instead of deadlocking. Fixes testcontainers#746 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
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
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.
Fixes #746
Reaper.get_instance()checked and setReaper._instancewithout a lock. Starting Ryuk takes a few seconds, so when two threads start containers at the same time, both see no instance, both createtestcontainers-ryuk-<SESSION_ID>, and the second create fails with a 409 Conflict. The repro in the issue (twoPostgresContainers from aThreadPoolExecutor) hits it.This adds the lock suggested in the issue, with a re-check inside it so threads that waited on the lock pick up the instance the first thread created. The fast path, where Ryuk is already running, still takes no lock.
The lock is an
RLockon purpose. WithTESTCONTAINERS_HUB_IMAGE_NAME_PREFIXset, starting Ryuk's own container callsget_instance()again on the same thread (#1085, PR #1120). Today that ends in a RecursionError; a plainLockwould turn it into a hang.Testing:
tests/core/test_ryuk.pystarts 8 threads on a barrier against a slow fake_create_instanceand asserts Ryuk is created once. It needs no Docker. It fails onmain(assert 8 == 1) and also fails if the lock is kept but the inner re-check removed.pre-commit runon the changed files (ruff, ruff-format, mypy) passes.pytest -m "not long_running" tests/corelocally on colima: 485 passed, 30 failed. The same 30 fail onmainon this machine (nodocker composeplugin, plustest_run_uses_found_network), none of them Reaper tests.