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

Prevent duplicate auth token activity updates #29357

Merged
merged 1 commit into from
Oct 22, 2021

Conversation

ChristophWurst
Copy link
Member

The auth token activity logic works as follows

  • Read auth token
  • Compare last activity time stamp to current time
  • Update auth token activity if it's older than x seconds

This works fine in isolation but with concurrency that means that
occasionally the same token is read simultaneously by two processes and
both of these processes will trigger an update of the same row.
Affectively the second update doesn't add much value. It might set the
time stamp to the exact same time stamp or one a few seconds later. But
the last activity is no precise science, we don't need this accuracy.

This patch changes the UPDATE query to include the expected value in a
comparison with the current data. This results in an affected row when
the data in the DB still has an old time stamp, but won't affect a row
if the time stamp is (nearly) up to date.

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.

How to test

  • Assign the result of $update->executeStatement(); to a var. It contains the number of affected rows.
  • Set a break point at the same line
  • Set the last_activity to 0 for the auth token that you want to test, e.g. the browser session token
  • Trigger some (ajax) requests and wait until more than one processes hit the break point
  • Take one step in each debugger session

You should see that the number of affected rows is 1 for the first process but 0 for the others.

@ChristophWurst ChristophWurst force-pushed the fix/concurrent-duplicate-auth-token-updates branch from 5c9ab5d to 238ac12 Compare October 21, 2021 13:11
The auth token activity logic works as follows
* Read auth token
* Compare last activity time stamp to current time
* Update auth token activity if it's older than x seconds

This works fine in isolation but with concurrency that means that
occasionally the same token is read simultaneously by two processes and
both of these processes will trigger an update of the same row.
Affectively the second update doesn't add much value. It might set the
time stamp to the exact same time stamp or one a few seconds later. But
the last activity is no precise science, we don't need this accuracy.

This patch changes the UPDATE query to include the expected value in a
comparison with the current data. This results in an affected row when
the data in the DB still has an old time stamp, but won't affect a row
if the time stamp is (nearly) up to date.

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.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst ChristophWurst force-pushed the fix/concurrent-duplicate-auth-token-updates branch from 238ac12 to 7dd7256 Compare October 22, 2021 07:32
@skjnldsv skjnldsv merged commit a9fe0fc into master Oct 22, 2021
@skjnldsv skjnldsv deleted the fix/concurrent-duplicate-auth-token-updates branch October 22, 2021 08:58
@ChristophWurst
Copy link
Member Author

/backport to stable22

@ChristophWurst
Copy link
Member Author

/backport to stable21

@ChristophWurst
Copy link
Member Author

Combined with #1037 this gives zero token updates. We did it @icewind1991! We did it! 😎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants