Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix race condition in FileSystemPackageRepository directory creation, modernize usage of os.makedirs #1913

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

nrusch
Copy link
Contributor

@nrusch nrusch commented Jan 28, 2025

The original impetus for this PR was to fix a race condition in FileSystemPackageRepository.pre_variant_install if multiple processes attempted to create a family directory in a narrow timing window.

From there, I decided to also go through and "modernize" any usage of os.makedirs to leverage the new exist_ok kwarg added in Python 3.2.

The PR is split up into 3 commits, in case you'd prefer to only integrate a subset:

  1. Fix the race condition.
  2. Replace rez.utils.filesystem.safe_makedirs with direct os.makedirs calls.
  3. Update any remaining uses of os.makedirs to use exist_ok=True where it seems appropriate.

@nrusch nrusch requested a review from a team as a code owner January 28, 2025 22:37
@nrusch
Copy link
Contributor Author

nrusch commented Jan 28, 2025

Thanks to @mgcollie for pointing out the original race condition.

Copy link

codecov bot commented Jan 28, 2025

Codecov Report

Attention: Patch coverage is 76.47059% with 4 lines in your changes missing coverage. Please review.

Project coverage is 59.31%. Comparing base (233b82c) to head (f482b34).

Files with missing lines Patch % Lines
src/rez/package_maker.py 50.00% 1 Missing ⚠️
src/rez/pip.py 0.00% 1 Missing ⚠️
src/rez/utils/filesystem.py 50.00% 1 Missing ⚠️
src/rez/utils/py_dist.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1913      +/-   ##
==========================================
+ Coverage   59.28%   59.31%   +0.02%     
==========================================
  Files         126      126              
  Lines       17218    17203      -15     
  Branches     3017     3008       -9     
==========================================
- Hits        10208    10204       -4     
+ Misses       6325     6316       -9     
+ Partials      685      683       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant