fix(rules_py): implement file-based locking for venv creation #680
Conversation
Add test to verify that multiple processes can safely use the same py_binary target simultaneously without encountering race conditions during venv creation. The test spawns 5 concurrent processes executing the same binary and validates that: - All processes complete successfully without errors - No import failures occur during concurrent venv initialization - No directory creation conflicts arise from simultaneous access This validates the file-locking mechanism that prevents race conditions when multiple instances try to create or access the same venv.
Prevent race conditions when multiple processes attempt to create or access the same Python venv simultaneously. Without locking, concurrent executions cause errors like 'cannot import package' and 'cannot create directory'. Implementation: - Add file-lock dependency (v2.1.11) for POSIX advisory locks via fcntl() - Introduce VenvLock struct that acquires exclusive locks on venv location - Lock blocks concurrent processes until available, auto-released on drop - Add is_venv_valid() to verify existing venvs are usable - Apply locking in both create_venv() and create_empty_venv() - Skip venv recreation if valid venv already exists When the second process acquires the lock, it detects the existing valid venv and skips recreation, eliminating race conditions entirely.
|
|
|
|
Hey @gibfahn, thanks for the PR. While you're of course welcome to hack this ruleset to fit your needs, this PR isn't aligned with the project roadmap which is to ditch the current dynamic venv machinery entirely in favor of prebuilt venvs (see the new My concern with implementing any solution like this is that -- see https://github.com/aspect-build/rules_py/pull/680/files#diff-71813fcee039f8c5ecbb0ea45a087af012c9e988132ce27075749cc2663f7968R368 -- it creates for the Since the only reason this implementation of Also tactically and with apologies I'm in the middle of ripping out the WORKSPACE file which involves overhauling (removing) many of the e2e tests so mechanically this isn't something I can just accept either. |
|
Fair enough, in that case I'll close this. Thank you for the detailed explanation. |
3 participants
test(rules_py): add E2E test for concurrent venv creation
Add test to verify that multiple processes can safely use the same
py_binary target simultaneously without encountering race conditions
during venv creation.
The test spawns 5 concurrent processes executing the same binary and
validates that:
This validates the file-locking mechanism that prevents race conditions
when multiple instances try to create or access the same venv.
fix(rules_py): implement file-based locking for venv creation
Prevent race conditions when multiple processes attempt to create or
access the same Python venv simultaneously. Without locking, concurrent
executions cause errors like 'cannot import package' and 'cannot create
directory'.
Implementation:
When the second process acquires the lock, it detects the existing valid
venv and skips recreation, eliminating race conditions entirely.