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

feat(Cache): make cache writes more atomic #9580

Merged
merged 3 commits into from
Jan 7, 2021

Conversation

rtm-ctrlz
Copy link
Contributor

Running parallel writes to cache may lead to race condition, when .tmp file is already renamed.

I'm getting error during tests via GitLab CI:

  • using docker-runners with shared composer cache (by mounting /var/composer/cache)
  • 3 parallel jobs (phpstan, phpcs, phpunit)
  • each make own composer install
  • usually all jobs pass install-phase without errors
  • sometimes one of jobs fails to install (see Random failures on CI related to file rename #9568)

May be there is other way:

  • to reproduce this race condition (it is necessary?)
  • workaround this problem with locks (ex. flock+timeout) - for me using locks is a proper way, but it would be a bit more complicated rather then just using more unique temporary filenames for every file/install.

Closes #9568

@mxr576
Copy link
Contributor

mxr576 commented Dec 30, 2020

Thanks @rtm-ctrlz for the patch and for confirming that the reported issues exist :)

I compiled a phar from the current state of this PR and testing it in our CI.

composer.phar.txt

@mxr576
Copy link
Contributor

mxr576 commented Dec 30, 2020

The first build was successful with the code in this PR, but the second failed with this error again. Maybe there is another race condition somewhere or this issue is unrelated?

#9568 (comment)

@mxr576
Copy link
Contributor

mxr576 commented Jan 5, 2021

Updated my findings in #9568, tl;dr; as of today, it looks to me this PR fixes the random failures caused by race conditions in cache writes.

src/Composer/Cache.php Outdated Show resolved Hide resolved
also updated `catch` block to use temporary filename
@rtm-ctrlz
Copy link
Contributor Author

@stof have a look, please

  • temporary files will have .tmp extension
  • also catch block will use correct filename

@Seldaek Seldaek merged commit e51e787 into composer:master Jan 7, 2021
@Seldaek
Copy link
Member

Seldaek commented Jan 7, 2021

Thanks, looks good to me.

@Seldaek Seldaek added this to the 2.0 milestone Jan 7, 2021
@Seldaek Seldaek added the Bug label Jan 7, 2021
@rtm-ctrlz rtm-ctrlz deleted the fix-9568 branch January 11, 2021 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Random failures on CI related to file rename
4 participants