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.

Sign up for GitHub

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

Jump to bottom

fix: Make authtoken activity updater simpler to avoid deadlocks #40971

Closed
wants to merge 1 commit into from
Closed

fix: Make authtoken activity updater simpler to avoid deadlocks #40971

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 0 additions & 23 deletions lib/private/Authentication/Token/PublicKeyTokenMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,28 +203,6 @@ public function hasExpiredTokens(string $uid): bool {
/**
* Update the last activity timestamp
*
* In highly concurrent setups it can happen that two parallel processes
* trigger the update at (nearly) the same time. In that special case it's
* not necessary to hit the database with two actual updates. Therefore the
* target last activity is included in the WHERE clause with a few seconds
* of tolerance.
*
* Example:
* - process 1 (P1) reads the token at timestamp 1500
* - process 1 (P2) reads the token at timestamp 1501
* - activity update interval is 100
*
* This means
*
* - P1 will see a last_activity smaller than the current time and update
* the token row
* - If P2 reads after P1 had written, it will see 1600 as last activity
* and the comparison on last_activity won't be truthy. This means no rows
* need to be updated a second time
* - If P2 reads before P1 had written, it will see 1501 as last activity,
* but the comparison on last_activity will still not be truthy and the
* token row is not updated a second time
*
* @param IToken $token
* @param int $now
*/
Expand All @@ -234,7 +212,6 @@ public function updateActivity(IToken $token, int $now): void {
->set('last_activity', $qb->createNamedParameter($now, IQueryBuilder::PARAM_INT))
->where(
$qb->expr()->eq('id', $qb->createNamedParameter($token->getId(), IQueryBuilder::PARAM_INT), IQueryBuilder::PARAM_INT),
$qb->expr()->lt('last_activity', $qb->createNamedParameter($now - 15, IQueryBuilder::PARAM_INT), IQueryBuilder::PARAM_INT)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a way to know current last_activity of the token?
If so, we could write this:

$qb->expr()->eq('last_activity', $qb->createNamedParameter($token->lastActivity(), IQueryBuilder::PARAM_INT), IQueryBuilder::PARAM_INT)

I think it can help to reduce locking

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

7dd7256

This is a micro optimization and will possibly not show any significant
performance improvement. Yet in setups with a DB cluster it means that
the write node has to send fewer changes to the read nodes due to the
lower number of actual changes.

This is exactly what I'm wondering about. Not sure if DB clusters are optimized enough to not resync changes when the query is "empty", but the idea was to avoid contacting the master.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Altahrim I thought about that too. We definitely have access to the previous last_activity value. But then I wonder if the additional clause in WHERE makes the query less or more prone to locking issues. The row is already looked up by the primary key. I can't imagine there being much difference between a less than and an equals check. They should be comparably fast/slow.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if DB clusters are optimized enough to not resync changes when the query is "empty"

If I recall correctly, Galera enforce row replication, so no change should lead to no replication sent.

The row is already looked up by the primary key.

I am agree. I would like to be sure it doesn't lock anything else because of the less than.
I generally saw equal instead of less than to check if row as been updated.

Maybe we can simply avoid this request PHP side if we detect a close enough timestamp?

);
$update->executeStatement();
}
Expand Down
Loading