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

lockForUpdate() method does not work. #406

Open
saernz opened this issue Jul 14, 2021 · 1 comment
Open

lockForUpdate() method does not work. #406

saernz opened this issue Jul 14, 2021 · 1 comment

Comments

@saernz
Copy link
Contributor

saernz commented Jul 14, 2021

Describe the bug
It seems the lockForUpdate() method available for laravel queries does not work when caching has been enabled and there is a cache hit. I have not had time to look further into this but I was having problems with a query not locking when it should have, and I found adding ->disableCache() to the query instantly fixed the issue. I believe if the cache gets a hit it returns the record and ignores the lockForUpdate() part of the query.

I think the proper behaviour when lockForUpdate() is encountered is to possibly automatically disable cache or use cache key locking if the cache provider implements Illuminate\Contracts\Cache\LockProvider contract.

Eloquent Query
An example of how to reproduce is below. In two different terminal sessions run artisan tinker, in one
session run the instance1 code, then in the other session while instance1 code is sleeping run instance2 code. When instance2 encounters the lock it should block until the lock is released, the issue I am seeing is that instance2 returns straight away rather than blocking.

function instance1() {
    DB::transaction(function () {
        SomeModel::where('id', 1)->lockForUpdate()->get();
        echo 'sleeping ';
        sleep(20);
    }, 2);
    echo time();
}
function instance2() {
    DB::transaction(function () {
        echo 'blocking ';
        SomeModel::where('id', 1)->lockForUpdate()->get();
    }, 2);
    echo time();
}

Environment

  • PHP: 7.2
  • Laravel: 7
  • Model Caching: 0.10.1
@joeyrush
Copy link

joeyrush commented Aug 6, 2021

In my opinion it should do your first suggestion of implicitly disabling cache since locking at the cache-level could lead to unexpected results (i.e. you could still perform updates on the 'locked' records since those could have been read pre-cache. This would then invalidate the cache and subsequent queries would bypass the cache lock etc.)

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

No branches or pull requests

2 participants