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

[8.x] Add lock support for file and null cache drivers #35139

Merged
merged 5 commits into from Nov 11, 2020
Merged

[8.x] Add lock support for file and null cache drivers #35139

merged 5 commits into from Nov 11, 2020

Conversation

paras-malhotra
Copy link
Contributor

@paras-malhotra paras-malhotra commented Nov 6, 2020

This PR adds a generic cache lock class that can implement a lock with any cache store implementation (kind of a bridge that takes a cache store and returns a cache lock). With this PR, we can now support locks for all cache drivers (including file, apc and null).

Laravel uses locks for several things including session blocking, WithoutOverlapping job middleware and unique jobs. This PR allows users to use these features for all cache drivers.

@paras-malhotra
Copy link
Contributor Author

@paras-malhotra paras-malhotra commented Nov 7, 2020

Also, added a NoLock for the null cache store driver. Similar to the Symfony NoLock, this lock can always be acquired. A use case could be when you want to conditionally make a job unique, you can use the uniqueVia method to return a NullStore for scenarios where the job shouldn't be unique.

The tests seem to be failing unrelated to this PR (in the install dependencies step). Perhaps a simple close/re-open bump might be needed.

@driesvints
Copy link
Member

@driesvints driesvints commented Nov 7, 2020

@paras-malhotra you can always re-run builds in the actions tab. I've re-triggered them for you.

@paras-malhotra
Copy link
Contributor Author

@paras-malhotra paras-malhotra commented Nov 7, 2020

Thanks @driesvints, didnt know we could do that. 👍 Tests are still failing for Windows, seems to be some issue with composer installing phpunit.

@taylorotwell
Copy link
Member

@taylorotwell taylorotwell commented Nov 7, 2020

Which drivers would have race conditions?

@paras-malhotra
Copy link
Contributor Author

@paras-malhotra paras-malhotra commented Nov 7, 2020

@taylorotwell, this PR makes the lock dependent on the cache store. A lock acquisition is simply a cache store get followed by a cache store put operation. So, the lock would be as robust as the store itself in terms of race conditions, meaning if there are no race conditions between 2 concurrent store put operations, there won't be race conditions between 2 concurrent lock acquisitions.

To be doubly sure, I examined the cache stores themselves. The file cache store uses shared locks for get and exclusive locks for put, so race conditions are taken care of. So, if any data changes between the get and put operations, the put will return false if the file lock is not acquired.

For the apc store, I'm not quite sure if there are race conditions between apc_store and apc_fetch but I reckon if the cache store is robust, the lock would also be robust. For NoLock there is no question of race conditions, because it simply returns true for acquiring the lock.

A case in point would be that the console scheduling cache event mutex operates in the same manner - skips if exists (get operation), creates on run (put operation) and forgets after run (delete operation). Similar to how a lock would work - attempt to acquire (get operation followed by put operation) and release after run (delete operation).

@taylorotwell
Copy link
Member

@taylorotwell taylorotwell commented Nov 9, 2020

I'm confused how there would not be a race condition in the file lock. Two processes are attempting to get the lock at about the same time. When both of them call get a very small amount of time apart... it returns null for both. One of them is slightly faster in reaching put and acquire the lock... then the next process catches up and calls put as well - stomping on the other process's call to put.

@paras-malhotra
Copy link
Contributor Author

@paras-malhotra paras-malhotra commented Nov 9, 2020

@taylorotwell, on a closer review, there is a chance of race conditions in the file lock. There are 2 scenarios:

  1. The second process calls put when the first put is not complete - this will result in the second process to fail acquiring the lock (no race condition) because file cache store put operations acquire exclusive locks, so the 2nd process will return false while trying to complete the put call.
  2. The second process calls put when the first put is complete - this will result in both processes acquiring the lock

Symfony uses flock for a more robust file lock implementation. I can either convert this PR to only submit the NoLock (lock for the null cache store) or we could implement everything and make a note in the docs that file and apc locks could result in race conditions (shouldn't be used in production). Let me know how we should proceed.

@taylorotwell
Copy link
Member

@taylorotwell taylorotwell commented Nov 9, 2020

You could potentially look at adding an add method to the FileStore cache driver that handles this in an atomic way if possible?

Regarding APC, it is probably the least used cache driver of all so I'm not particularly concerned about if we can't support locks on that driver.

@paras-malhotra paras-malhotra marked this pull request as draft Nov 9, 2020
@paras-malhotra paras-malhotra marked this pull request as ready for review Nov 10, 2020
@paras-malhotra
Copy link
Contributor Author

@paras-malhotra paras-malhotra commented Nov 10, 2020

@taylorotwell, I've added the add method on the FileStore cache driver that does an atomic put if the file doesn't exist or has expired. Currently, the Illuminate\Filesystem\Filesystem methods (e.g. get and put) didn't support operations on a single file resource. So, to handle this in an atomic manner, I had to create a File class to operate on a single resource with an exclusive lock for both read and write ops.

I think we are ready to merge this in. Let me know if you'd like me to make any additional changes.

@paras-malhotra paras-malhotra changed the title [8.x] Add lock support for all cache drivers [8.x] Add lock support for file and null cache drivers Nov 10, 2020
@taylorotwell taylorotwell merged commit 6cb85b2 into laravel:8.x Nov 11, 2020
13 checks passed
@paras-malhotra paras-malhotra deleted the lock_bridge branch Nov 11, 2020
@talovicnedim
Copy link

@talovicnedim talovicnedim commented Jan 18, 2021

@paras-malhotra @taylorotwell

Is it possible the file driver fails to lock or something like that? For example, I have the following:

  public $uniqueFor = 3600;

  protected int $id;

  public function __construct(int $id)
  {
      $this->id = $id;
  }

  public function uniqueId()
  {
      return $this->id;
  }

and ShouldBeUnique of course.

However, sometimes it isn't unique. This doesn't happen often, but sometimes it happens. I switched to Redis so we'll see if that will happen with Redis too.

Thanks!

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