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

[10.x] Fix race condition in locks issued by the file cache driver #43686

Closed

Conversation

hbgl
Copy link

@hbgl hbgl commented Aug 12, 2022

This PR fixes issue #43627.
I previously submitted similar changes to branch 9.x in PR #43661.

TLDR:

Atomic locks issued by FileStore or other stores that use the HasCacheLock trait have a race condition when the lock timeout is zero (the default). This can cause multiple processes to concurrently enter a critical section protected by the lock.

The fix:

The old lock implementation would do a non-atomic get and put when the lock timeout is less than or equal to zero and an atomic add otherwise. The new implementation always does an atomic add. This change only breaks code that incorrectly implemented their cache store, i.e. their add method cannot handle lock timeouts less than or equal to zero.

@driesvints driesvints changed the title Fix race condition in locks issued by the file cache driver [10.x] Fix race condition in locks issued by the file cache driver Aug 15, 2022
@taylorotwell
Copy link
Member

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If possible, please consider releasing your code as a package so that the community can still take advantage of your contributions!

If you feel absolutely certain that this code corrects a bug in the framework, please "@" mention me in a follow-up comment with further explanation so that GitHub will send me a notification of your response.

@driesvints
Copy link
Member

@hbgl I guess Taylor doesn't wants to change this sorry. Maybe try a PR to the docs to document the limitation?

@eli-s-r
Copy link

eli-s-r commented Dec 2, 2022

@taylorotwell I do believe this is a bug worth merging this PR for, as this race condition renders the atomic lock on the file cache useless for the default case where a fixed amount of time for the lock isn't desired and high amounts of concurrency are expected.

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.

None yet

4 participants