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 MkdirFunc #4404

Merged
merged 1 commit into from
Sep 6, 2023
Merged

Fix race condition in MkdirFunc #4404

merged 1 commit into from
Sep 6, 2023

Conversation

vit9696
Copy link
Contributor

@vit9696 vit9696 commented Sep 2, 2023

With variant dirs SCons creates directories in the build directory per each SCons file. And this can race when parallel SCons tasks are launched in the same directory even if they have separate targets and sconsign files.

The issue code in question is as follows:

  • _SConscript performs directory creation.
  • It calls Dir's _create() method
  • and that one reaches MkdirFunc through MkdirBuilder (SCons.Node.Node.build())

MkdirFunc function already checks for t.exists(), so as discussed on the chat a workaround to make this function atomic should be more or less ok.

Contributor Checklist:

  • I have created a new test or updated the unit tests to cover the new/changed functionality.
  • I have updated CHANGES.txt (and read the README.rst)
  • I have updated the appropriate documentation

No documentation or test changes should be necessary for this commit.

CC @mwichmann

@mwichmann
Copy link
Collaborator

Note the one Windows test suite failure is unrelated to the change, it's a different race, which #4402 was just merged to help with - though that came too late for this build.

[00:21:32] JSONDecodeError: Expecting value: line 1 column 1 (char 0):
[00:21:32]   File "C:\Users\appveyor\AppData\Local\Temp\1\testcmd.1860.nq3xr9bv\SConstruct", line 3:
[00:21:32]     env = Environment(MSVC_VERSION='14.3', MSVC_TOOLSET_VERSION='14.3', tools=['msvc'])
[00:21:32]   File "C:\projects\scons\SCons\Environment.py", line 1244:

SCons/Node/FS.py Outdated Show resolved Hide resolved
@mwichmann
Copy link
Collaborator

As mentioned before this became a PR, this looks simple and straightforward to me.

CHANGES.txt Outdated Show resolved Hide resolved
@bdbaddog
Copy link
Contributor

bdbaddog commented Sep 5, 2023

Looks good to me.
Can you add same blurb from CHANGES.txt to RELEASE.txt?

Also, entries in CHANGES.txt are generally sorted per release by the contributors last name.
Can you move your update to CHANGES.txt to be ordered as such and not at the end of the changes for the current (working) release?

Thanks for this fix!

@vit9696
Copy link
Contributor Author

vit9696 commented Sep 5, 2023

Done. Also rebased and squashed to fix type annotation for the function header.

@bdbaddog bdbaddog merged commit c2af6ac into SCons:master Sep 6, 2023
4 of 5 checks passed
@mwichmann mwichmann added this to the 4.6 milestone Sep 6, 2023
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.

3 participants