Page MenuHomePhabricator

Deprecate User::getInstanceForUpdate()
Closed, ResolvedPublic

Description

Since rMW3a52a5fe87af: User: Reduce locking severity of ::getInstanceForUpdate() it doesn't exactly return an instance for update. It should be replaced with a "get primary instance" method and maybe "get primary instance with shared lock" / "get primary instance with exclusive lock" methods, if those are really needed. Most call sites seem to only change user options which is a separate table; I don't think there is a point in locking the user table row for that.

call sites

See also:
T405225: Selenium failures due to "Error 1213 from MediaWiki\Deferred\UserEditCountUpdate::doUpdate, Deadlock found when trying to get lock; try restarting transaction"
{T405112}

Details

Related Changes in Gerrit:
SubjectRepoBranchLines +/-
mediawiki/coremaster+5 -0
mediawiki/coremaster+6 -2
mediawiki/coreREL1_45+6 K -0
mediawiki/extensions/GrowthExperimentsmaster+12 -9
mediawiki/coremaster+17 -17
mediawiki/extensions/UniversalLanguageSelectormaster+3 -4
mediawiki/extensions/IPInfomaster+2 -2
mediawiki/extensions/GrowthExperimentsmaster+2 -3
mediawiki/extensions/ContentTranslationmaster+0 -2
mediawiki/extensions/TranslationNotificationsmaster+5 -6
mediawiki/extensions/TheWikipediaLibrarymaster+0 -1
mediawiki/extensions/EnhancedStandardUIsmaster+2 -3
mediawiki/extensions/GlobalPreferencesmaster+14 -16
mediawiki/coremaster+1 -3
mediawiki/extensions/MobileFrontendmaster+4 -4
mediawiki/extensions/BetaFeaturesmaster+2 -5
mediawiki/extensions/LastUserLoginmaster+0 -0
mediawiki/coremaster+3 -11
mediawiki/extensions/BetaFeaturesmaster+2 -1
mediawiki/coremaster+11 -7
mediawiki/extensions/MobileFrontendmaster+2 -2
mediawiki/coremaster+39 -43
mediawiki/extensions/GlobalPreferencesmaster+10 -10
mediawiki/coremaster+39 -5
mediawiki/corewmf/1.45.0-wmf.20+39 -5
Show related patches Customize query in gerrit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Change #1190625 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/core@master] Deprecate User::getInstanceForUpdate()

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

Change #1190625 merged by jenkins-bot:

[mediawiki/core@master] Deprecate User::getInstanceForUpdate()

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

Change #1190649 had a related patch set uploaded (by Ladsgroup; author: Gergő Tisza):

[mediawiki/core@wmf/1.45.0-wmf.20] Deprecate User::getInstanceForUpdate()

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

Change #1190649 merged by jenkins-bot:

[mediawiki/core@wmf/1.45.0-wmf.20] Deprecate User::getInstanceForUpdate()

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

Mentioned in SAL (#wikimedia-operations) [2025-09-23T12:16:28Z] <ladsgroup@deploy1003> Started scap sync-world: Backport for [[gerrit:1190649|Deprecate User::getInstanceForUpdate() (T405231)]]

Mentioned in SAL (#wikimedia-operations) [2025-09-23T12:22:35Z] <ladsgroup@deploy1003> ladsgroup: Backport for [[gerrit:1190649|Deprecate User::getInstanceForUpdate() (T405231)]] synced to the testservers (see https://wikitech.wikimedia.org/wiki/Mwdebug). Changes can now be verified there.

Mentioned in SAL (#wikimedia-operations) [2025-09-23T12:33:22Z] <ladsgroup@deploy1003> Finished scap sync-world: Backport for [[gerrit:1190649|Deprecate User::getInstanceForUpdate() (T405231)]] (duration: 16m 54s)

Change #1190710 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/core@master] Do not lock user table in options APIs

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

Change #1190711 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/extensions/GlobalPreferences@master] Do not lock user table in global options APIs

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

Change #1190710 merged by jenkins-bot:

[mediawiki/core@master] Do not lock user table in options APIs

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

Change #1190711 merged by jenkins-bot:

[mediawiki/extensions/GlobalPreferences@master] Do not lock user table in global options APIs

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

Change #1191818 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/core@master] Replace User::getInstanceForUpdate()

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

Change #1191819 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/extensions/BetaFeatures@master] Replace User::getInstanceForUpdate()

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

Change #1191820 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/extensions/MobileFrontend@master] Do not save entire user record just to update preferences

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

Change #1191818 merged by jenkins-bot:

[mediawiki/core@master] Replace User::getInstanceForUpdate()

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

Change #1191820 merged by jenkins-bot:

[mediawiki/extensions/MobileFrontend@master] Do not save entire user record just to update preferences

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

Replace User::getInstanceForUpdate()

[…] Do not save user row in Special:Preferences when we only want
to change user options; and do not lock.

@Tgr Note that User::saveSettings is also responsible for bumping user_touched in the user row, which ensures that preferences are consistently applied on subsequent page views. Including whether repeat pageviews will renew (HTTP 304) or replace (HTTP 200) based on the Last-Modified header.

ChronologyProtector does not solve this, because if the server does not even generate the page, then the up-to-date-ness of database values does not help us displace outdated values in the browser cache.

Fortunately, $this->userOptionsManager->saveOptions( $user ); actually calls $legacyUser->checkAndSetTouched(); so we still touch the user row as we should.

Test prep:

  • Edit UserOptionsManager.php#saveOptions and comment-out the $legacyUser->checkAndSetTouched(); line.
  • Open http://localhost:4000/index.php?title=Main_Page&action=history, open network devtools, make sure "Disable cache" is not on.
  • Wait two minutes, or remove the minute-long cpPosIndex=N and UseDC=master cookies.

Test:

  1. Click on "View history" to naturally reload the page. Observe HTTP 304 response. OK.
  2. In another tab, open preferences and change language to Dutch, save.
  3. Wait two minutes, or remove the cpPosIndex/UseDC cookies.
  4. Click on "View history" in the first tab, observe HTTP 200 response. OK
  5. Click on "View history" in the first tab, observe HTTP 304 response. OK
  6. In the other tab, open preferences, and do a full reset preferences (includes language back to English).
  7. Wait two minutes, or remove the cpPosIndex/UseDC cookies.
  8. Click on "View history" in the first tab, observe HTTP 304 response. Bad! — Page is displayed in Dutch instead of English.

Change #1191819 merged by jenkins-bot:

[mediawiki/extensions/BetaFeatures@master] Replace User::getInstanceForUpdate()

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

I forgot to rename a bunch of getInstanceForUpdate mocks in EmailNotificationSecondaryAuthenticationProviderTest, and it's passing anyway, so maybe it's not actually testing what it's supposed to.

Change #1206353 had a related patch set uploaded (by D3r1ck01; author: Derick Alangi):

[mediawiki/core@master] tests: Avoid usage of deprecated User::getInstanceForUpdate

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

I forgot to rename a bunch of getInstanceForUpdate mocks in EmailNotificationSecondaryAuthenticationProviderTest, and it's passing anyway, so maybe it's not actually testing what it's supposed to.

I did them just in case: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1206353.

Apart from https://codesearch.wmcloud.org/deployed/?q=getInstanceForUpdate&files=&excludeFiles=&repos=, is there anything else left to be done here?

Change #1206353 merged by jenkins-bot:

[mediawiki/core@master] tests: Avoid usage of deprecated User::getInstanceForUpdate

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

Change #1211802 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/core@master] Do not bump user_touched after user options save

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

Change #1211804 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/extensions/BetaFeatures@master] Do not fetch a primary User instance just for saving preferences

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

Change #1211806 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/extensions/GlobalPreferences@master] Do not fetch a primary User instance just for saving preferences

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

Change #1211807 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/extensions/MobileFrontend@master] Do not fetch a primary User instance just for saving preferences

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

Change #1211808 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/core@master] Do not use a read lock when changing email address

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

Change #1213112 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/extensions/UniversalLanguageSelector@master] Do not fetch a primary User instance just for saving preferences

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

Change #1213113 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/extensions/TheWikipediaLibrary@master] Do not fetch a primary User instance just for saving preferences

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

Change #1213114 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/extensions/LastUserLogin@master] Do not fetch a primary User instance just for saving preferences

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

Change #1213114 abandoned by Gergő Tisza:

[mediawiki/extensions/LastUserLogin@master] Do not fetch a primary User instance just for saving preferences

Reason:

oops, wrong repo

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

Change #1213115 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/extensions/IPInfo@master] Do not fetch a primary User instance just for saving preferences

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

Change #1213116 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/extensions/EnhancedStandardUIs@master] Do not fetch a primary User instance just for saving preferences

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

Change #1213117 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/extensions/TranslationNotifications@master] Do not fetch a primary User instance just for saving preferences

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

Change #1213118 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/extensions/ContentTranslation@master] Do not fetch a primary User instance just for saving preferences

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

Change #1213119 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/extensions/GrowthExperiments@master] Do not fetch a primary User instance just for saving preferences

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

Change #1211802 merged by jenkins-bot:

[mediawiki/core@master] user: Handle replag in UserOptionsManager::saveOptions()

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

Change #1211807 merged by jenkins-bot:

[mediawiki/extensions/MobileFrontend@master] Do not fetch a primary User instance just for saving preferences

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

Change #1211806 merged by jenkins-bot:

[mediawiki/extensions/GlobalPreferences@master] Do not fetch a primary User instance just for saving preferences

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

Change #1211804 merged by jenkins-bot:

[mediawiki/extensions/BetaFeatures@master] Do not fetch a primary User instance just for saving preferences

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

Change #1211808 merged by jenkins-bot:

[mediawiki/core@master] Do not use a read lock when changing email address

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

Change #1213116 merged by jenkins-bot:

[mediawiki/extensions/EnhancedStandardUIs@master] Do not fetch a primary User instance just for saving preferences

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

Change #1213113 merged by jenkins-bot:

[mediawiki/extensions/TheWikipediaLibrary@master] Do not fetch a primary User instance just for saving preferences

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

Change #1213118 merged by jenkins-bot:

[mediawiki/extensions/ContentTranslation@master] Do not fetch a primary User instance just for saving preferences

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

Change #1213117 merged by jenkins-bot:

[mediawiki/extensions/TranslationNotifications@master] Do not fetch a primary User instance just for saving preferences

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

Change #1213115 merged by jenkins-bot:

[mediawiki/extensions/IPInfo@master] Do not fetch a primary User instance just for saving preferences

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

Change #1213119 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] Do not fetch a primary User instance just for saving preferences

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

Change #1213112 merged by jenkins-bot:

[mediawiki/extensions/UniversalLanguageSelector@master] Do not fetch a primary User instance just for saving preferences

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

Change #1214134 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/GrowthExperiments@master] Replace remaining uses of deprecated User::getInstanceForUpdate()

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

Change #1214137 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@master] User: Hard-deprecate getInstanceForUpdate()

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

These are the last two patches for this task, there are no other uses in Wikimedia-deployed code: https://codesearch.wmcloud.org/deployed/?q=getInstanceForUpdate

Change #1214134 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] Replace remaining uses of deprecated User::getInstanceForUpdate()

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

Change #1214137 merged by jenkins-bot:

[mediawiki/core@master] User: Hard-deprecate getInstanceForUpdate()

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

Change #1218831 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/core@master] Fix deprecation notice for User::getInstanceForUpdate()

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

Change #1218832 had a related patch set uploaded (by Gergő Tisza; author: Bartosz Dziewoński):

[mediawiki/core@REL1_45] User: Hard-deprecate getInstanceForUpdate()

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

Change #1218832 abandoned by Gergő Tisza:

[mediawiki/core@REL1_45] User: Hard-deprecate getInstanceForUpdate()

Reason:

I suppose we shouldn't backport deprecations anyway.

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

Change #1218831 merged by jenkins-bot:

[mediawiki/core@master] Fix deprecation notice for User::getInstanceForUpdate()

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