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

Updating last_login_at leads to deadlocks #207

Closed
MatanYadaev opened this issue Oct 15, 2020 · 3 comments
Closed

Updating last_login_at leads to deadlocks #207

MatanYadaev opened this issue Oct 15, 2020 · 3 comments

Comments

@MatanYadaev
Copy link
Contributor

MatanYadaev commented Oct 15, 2020

Due to a high concurrency on my company's API, Sanctum leads to deadlocks.
This line of code, which updating last_login_at is responsible for these deadlocks.

tap($accessToken->forceFill(['last_used_at' => now()]))->save()

My company doesn't need this last_login_at column, and I can see that Sanctum doesn't use it anywhere.
It seems like an optional column. I guess it exists just for those who'd like to display it to the users or to make some logic above it.

I think Sanctum should provide the ability to choose whether this column is "working" or not. It should be configurable in my opinion.

What do you think guys?


As a workaround, I've used an observer with a return false to cancel this update query.

<?php

namespace App\Observers;

use Laravel\Sanctum\PersonalAccessToken;

class PersonalAccessTokenObserver
{
    public function updating(PersonalAccessToken $accessToken)
    {
        $dirtyAttributes = array_keys($accessToken->getDirty());

        if (count($dirtyAttributes) === 1 && $dirtyAttributes[0] === 'last_used_at') {
            return false;
        }
    }
}
@hananbo
Copy link

hananbo commented Oct 15, 2020

Would like to add to my colleague @MatanYadaev arguments and clarify the trade-off of maintaining last_used_at column:

Each and every authenticated request is followed by both READ and WRITE operations on the personal_access_tokens table:. 1. query the token, 2. update the last_used_at.

A common scenario of a dashboard page load which simultaniosly fires multiple requests to load different dashboard data, is enough to hit concurrency level that leads to deadlock occurences on the personal_access_tokens table due to relatively high 1:1 READ to WRITE ratio.

Ofcourse some DB parameter adjustments can be made to mitigate this issue. However, I think reducing the stress on this table by simply making tracking of last_used_at configurable can help making this great package appealing for projects with slightly more intensive requests volume and therfore is a worthy consideration.

will appreciate your opinion here before we start working on a suggested PR.

@driesvints
Copy link
Member

Please resubmit this with a filled out issue template. It's there for a reason.

@ankurk91
Copy link
Contributor

You can override the default model like

// in your AppServiceProvider
Sanctum::usePersonalAccessTokenModel(PersonalAccessToken::class);
<?php

namespace App\Models;

use Laravel\Sanctum\PersonalAccessToken as SanctumPersonalAccessToken;

class PersonalAccessToken extends SanctumPersonalAccessToken
{
    public static function booted()
    {
        // Prevent updating the last_used_at column on every request
        static::updating(function (self $accessToken) {
            $dirty = $accessToken->getDirty();

            if (count($dirty) === 1 && isset($dirty['last_used_at'])) {
                return false;
            }

            return true;
        });
    }
}

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

4 participants