Should UserOptionsManager::saveOptions() be @internal ?
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	DannyS712
	Jun 4 2021, 9:56 PM

Description

Its currently marked as internal, but as places have been updated to use UserOptionsManager::setOption() instead of User::setOption() some calls to User::saveSettings() have been replaced with UserOptionsManager::saveOptions() too - is this the correct thing to do?

Details

Related Changes in Gerrit:

Subject	Repo	Branch	Lines +/-
Reapply "Update user_touched after saving user options."	mediawiki/core	master	+83 -18
Update user_touched after saving user options.	mediawiki/core	master	+83 -18
Protect UserOptionsManager::saveOptions with user CAS	mediawiki/core	master	+38 -10

Customize query in gerrit

Related Objects

Mentioned In: T294265: CentralAuthUserUsingDatabaseTest::testAdminLockAndHide fails
Mentioned Here: T294265: CentralAuthUserUsingDatabaseTest::testAdminLockAndHide fails
T280220: Error "Lock wait timeout exceeded" from User::loadFromDatabase (via API action=options)

Event Timeline

DannyS712 created this task.Jun 4 2021, 9:56 PM

Restricted Application added a project: User-DannyS712. · View Herald TranscriptJun 4 2021, 9:56 PM

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

DannyS712 moved this task from Unsorted to Reports on the User-DannyS712 board.Jun 4 2021, 9:57 PM

DannyS712 added a subscriber: • Pchelolo.

Zabe subscribed.Jun 4 2021, 10:31 PM

@Pchelolo , thoughts on this? Not sure how to triage it, but if you have a quick answer maybe we can just answer and close it.

Yeah, I think that would be the right thing to do.

User::saveSettings() updates the touched timestamp of the user, UserOptionsManager::saveOptions() does not. It's used in the caching logic to avoid returning a 304 when the user changed preferences relevant to rendering the page. The public version of saveOptions() should probably call User::checkAndSetTouched().

Oh, good point. Will do.

Also, the touched timestamp is a CAS mechanism. So something like

$user->setOption( 'foo', $user->getOption( 'foo' ) + 1 );
$user->saveOptions();

is guaranteed to increment exactly by 1 (or throw a CAS error if the $user object has outdated data at the time of saving). With the similar UserOptionsManager methods, that isn't the case.

All the points raised by @Tgr are very valid, apologies for writing it's a right thing to do before considering those. I think though it should be made into the right thing to do - User::saveSettings re-writes all user fields into a user table, which with exception of user_touched are entirely unnecessary. Also this creates a coupling between a manager and User.

I've started a refactoring of User::saveOptions here https://gerrit.wikimedia.org/r/c/mediawiki/core/+/699466/2 - these address performance issues. I'll do some more refactoring to introduce CAS to user options saving and then remove the @internal mark.

• Pchelolo edited projects, added Platform Team Workboards (MW Expedition); removed Platform Engineering.Jun 14 2021, 5:30 PM

• Pchelolo moved this task from Unsorted pile to Waiting for Review on the Platform Team Workboards (MW Expedition) board.Jun 21 2021, 9:02 PM

Change 700701 had a related patch set uploaded (by Ppchelko; author: Ppchelko):

[mediawiki/core@master] Protect UserOptionsManager::saveOptions with user CAS

https://gerrit.wikimedia.org/r/700701

gerritbot added a project: Patch-For-Review.Jun 21 2021, 9:06 PM

In T284354#7154068, @Tgr wrote:
Also, the touched timestamp is a CAS mechanism. So something like
$user->setOption( 'foo', $user->getOption( 'foo' ) + 1 );
$user->saveOptions();
is guaranteed to increment exactly by 1 (or throw a CAS error if the $user object has outdated data at the time of saving). With the similar UserOptionsManager methods, that isn't the case.

I think making this a hard requirement conflicts with solving T280220. If we try to make every data store serve every conceivable access pattern, we'll end up with an overcomplicated, badly performing compromise. The most important thing is that requests complete in a reasonable amount of time without errors.

For example, Special:Mute is packing an array into a single user option and is modifying the array. If a given user posts concurrent updates to Special:Mute, we can either wait for a DB lock, potentially taking the site down, or we can let updates overwrite each other. Considering typical access patterns to this special page, I would prefer to let updates overwrite each other. If it turns out that users are constantly posting to Special:Mute concurrently and they wish their updates wouldn't overwrite each other, the correct solution for that is to use a separate table, and to map mute and unmute actions to delete and insert queries on that table.

Change 700701 abandoned by Ppchelko:

[mediawiki/core@master] Protect UserOptionsManager::saveOptions with user CAS

Reason:

No need to do this.

https://gerrit.wikimedia.org/r/700701

Maintenance_bot removed a project: Patch-For-Review.Jul 16 2021, 3:10 AM

Change 705150 had a related patch set uploaded (by Ppchelko; author: Ppchelko):

[mediawiki/core@master] Update user_touched after saving user options

https://gerrit.wikimedia.org/r/705150

gerritbot added a project: Patch-For-Review.Jul 17 2021, 3:09 PM

Krinkle subscribed.Sep 16 2021, 9:40 PM

Change 705150 merged by jenkins-bot: