mikebronner / laravel-model-caching

Eloquent model-caching made easy.
MIT License
2.23k stars 212 forks source link

lockForUpdate() method does not work. #406

Open saernz opened 3 years ago

saernz commented 3 years ago

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

joeyrush commented 2 years ago

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.)