Page MenuHomePhabricator

Recursive service instantiation: Circular dependency when creating service! MainWANObjectCache
Closed, ResolvedPublic

Description

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/GrowthExperiments/+/685544
https://integration.wikimedia.org/ci/job/quibble-vendor-mysql-php72-noselenium-docker/82325/console

20:17:44 1) SpecialPageFatalTest::testSpecialPageDoesNotFatal with data set "MultiLock" ('MultiLock')
20:17:44 Wikimedia\Services\RecursiveServiceDependencyException: Recursive service instantiation: Circular dependency when creating service! GrowthExperimentsMultiConfig -> GrowthExperimentsWikiPageConfig -> GrowthExperimentsWikiPageConfigLoader -> RevisionLookup -> RevisionStore -> RevisionStoreFactory -> BlobStoreFactory -> MainWANObjectCache -> UserNameUtils -> TitleParser -> _MediaWikiTitleCodec -> InterwikiLookup -> MainWANObjectCache

Core CI seems fine, so I assume this is Growth-specific, although no idea how.

Stack trace:

20:49:51 1) SpecialPageFatalTest::testSpecialPageDoesNotFatal with data set "MultiLock" ('MultiLock')
20:49:51 Wikimedia\Services\RecursiveServiceDependencyException: Recursive service instantiation: Circular dependency when creating service! GrowthExperimentsMultiConfig -> GrowthExperimentsWikiPageConfig -> GrowthExperimentsWikiPageConfigLoader -> RevisionLookup -> RevisionStore -> RevisionStoreFactory -> BlobStoreFactory -> MainWANObjectCache -> UserNameUtils -> TitleParser -> _MediaWikiTitleCodec -> InterwikiLookup -> MainWANObjectCache
20:49:51 
20:49:51 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:437
20:49:51 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:416
20:49:51 /workspace/src/includes/MediaWikiServices.php:266
20:49:51 /workspace/src/includes/MediaWikiServices.php:1063
20:49:51 /workspace/src/includes/ServiceWiring.php:578
20:49:51 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:447
20:49:51 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:416
20:49:51 /workspace/src/includes/MediaWikiServices.php:266
20:49:51 /workspace/src/includes/MediaWikiServices.php:908
20:49:51 /workspace/src/includes/ServiceWiring.php:1722
20:49:51 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:447
20:49:51 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:416
20:49:51 /workspace/src/includes/MediaWikiServices.php:266
20:49:51 /workspace/src/includes/ServiceWiring.php:1487
20:49:51 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:447
20:49:51 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:416
20:49:51 /workspace/src/includes/MediaWikiServices.php:266
20:49:51 /workspace/src/includes/MediaWikiServices.php:1499
20:49:51 /workspace/src/includes/ServiceWiring.php:1568
20:49:51 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:447
20:49:51 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:416
20:49:51 /workspace/src/includes/MediaWikiServices.php:266
20:49:51 /workspace/src/includes/MediaWikiServices.php:1579
20:49:51 /workspace/src/includes/user/User.php:2070
20:49:51 /workspace/src/includes/user/User.php:2938
20:49:51 /workspace/src/includes/user/User.php:2956
20:49:51 /workspace/src/includes/session/SessionBackend.php:669
20:49:51 /workspace/src/includes/session/SessionBackend.php:234
20:49:51 /workspace/src/includes/session/Session.php:73
20:49:51 /workspace/src/includes/ServiceWiring.php:810
20:49:51 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:447
20:49:51 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:416
20:49:51 /workspace/src/includes/MediaWikiServices.php:266
20:49:51 /workspace/src/includes/MediaWikiServices.php:1063
20:49:51 /workspace/src/includes/ServiceWiring.php:210
20:49:51 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:447
20:49:51 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:416
20:49:51 /workspace/src/includes/MediaWikiServices.php:266
20:49:51 /workspace/src/includes/MediaWikiServices.php:611
20:49:51 /workspace/src/includes/ServiceWiring.php:1266
20:49:51 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:447
20:49:51 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:416
20:49:51 /workspace/src/includes/MediaWikiServices.php:266
20:49:51 /workspace/src/includes/MediaWikiServices.php:1338
20:49:51 /workspace/src/includes/ServiceWiring.php:1250
20:49:51 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:447
20:49:51 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:416
20:49:51 /workspace/src/includes/MediaWikiServices.php:266
20:49:51 /workspace/src/includes/MediaWikiServices.php:1330
20:49:51 /workspace/src/includes/ServiceWiring.php:1236
20:49:51 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:447
20:49:51 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:416
20:49:51 /workspace/src/includes/MediaWikiServices.php:266
20:49:51 /workspace/src/includes/MediaWikiServices.php:1314
20:49:51 /workspace/src/extensions/GrowthExperiments/ServiceWiring.php:479
20:49:51 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:447
20:49:51 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:416
20:49:51 /workspace/src/includes/MediaWikiServices.php:266
20:49:51 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:424
20:49:51 /workspace/src/extensions/GrowthExperiments/includes/GrowthExperimentsServices.php:184
20:49:51 /workspace/src/extensions/GrowthExperiments/ServiceWiring.php:99
20:49:51 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:447
20:49:51 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:416
20:49:51 /workspace/src/includes/MediaWikiServices.php:266
20:49:51 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:424
20:49:51 /workspace/src/extensions/GrowthExperiments/includes/GrowthExperimentsServices.php:180
20:49:51 /workspace/src/extensions/GrowthExperiments/ServiceWiring.php:89
20:49:51 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:447
20:49:51 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:416
20:49:51 /workspace/src/includes/MediaWikiServices.php:266
20:49:51 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:424
20:49:51 /workspace/src/vendor/wikimedia/object-factory/src/ObjectFactory/ObjectFactory.php:209
20:49:51 /workspace/src/vendor/wikimedia/object-factory/src/ObjectFactory/ObjectFactory.php:150
20:49:51 /workspace/src/includes/HookContainer/HookContainer.php:465
20:49:51 /workspace/src/includes/HookContainer/HookContainer.php:156
20:49:51 /workspace/src/includes/HookContainer/HookRunner.php:3677
20:49:51 /workspace/src/includes/specialpage/SpecialPageFactory.php:1099
20:49:51 /workspace/src/includes/specialpage/SpecialPageFactory.php:1114
20:49:51 /workspace/src/includes/specialpage/SpecialPageFactory.php:1172
20:49:51 /workspace/src/includes/specialpage/SpecialPageFactory.php:1202
20:49:51 /workspace/src/tests/phpunit/structure/SpecialPageFatalTest.php:40
20:49:51 /workspace/src/tests/phpunit/MediaWikiIntegrationTestCase.php:448

Event Timeline

No idea what's happening here. The error seems to imply that the service factory function for MainWANObjectCache requests UserNameUtils from the service container, but it doesn't (and there is no reason why it would). The rest of the dependencies seem legit.

MultiLock is a CentralAuth special page so probably this is some conflict with that extension (which does use UserNameUtils a lot).

Locally, there's no issue, with CentralAuth and GrowthExperiments installed.

vagrant@growth:~$ mwscript /vagrant/mediawiki/tests/phpunit/phpunit.php --wiki=wiki --filter MultiLock /vagrant/mediawiki/tests/phpunit/structure/SpecialPageFatalTest.php 
#!/usr/bin/env php
Using PHP 7.2.31-1+0~20200514.41+debian9~1.gbpe2a56b+wmf1
PHPUnit 8.5.15 by Sebastian Bergmann and contributors.

.                                                                   1 / 1 (100%)

Time: 3.49 seconds, Memory: 56.25 MB

OK (1 test, 1 assertion)
Tgr triaged this task as Unbreak Now! priority.May 9 2021, 8:25 PM

Started happening some time yesterday (last good check is 0:42 UTC, first bad check is 20:25).

No merges in the relevant time window in CentralAuth. None of the core merges look relevant. rEGREb4b9beb392da: Use static closures where safe to use seems a bit suspicious (in theory it should be a no-op but it changes ServiceWiring and might trigger some obscure bug) but it's a bit too early, it was merged at 18h the day before yesterday, with several patches merged afterwards. The last successful merge was rEGRE2f2d084f5ae9: Tracker: Untrack pages after link recommendation edit which is definitely unrelated. A CI config change, maybe?

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

[mediawiki/extensions/GrowthExperiments@master] [DNM] CI test

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

Hm, got a fully stack trace?

I suspect this involves ObjectCache::newFromParams. It seems that one of the ObjectCaches used by WANObjectCache apparently needs UserNameUtils...

Hm, got a fully stack trace?

I suspect this involves ObjectCache::newFromParams. It seems that one of the ObjectCaches used by WANObjectCache apparently needs UserNameUtils...

Found the stack trace. ObjectCache is not on the stack. Instead, the recursion happens via the destructor of a Session, which is triggered when the scope of the wiring method for WANObjectCache ends. It's not obvious why a (new local) Session instance would be created in that scope.

Change 688264 had a related patch set uploaded (by Daniel Kinzler; author: Daniel Kinzler):

[mediawiki/core@master] [DNM] Force stack trace from Session constructor

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

For the time being, we could work around the issue by making UserNameUtils::isIP static.

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

[mediawiki/core@master] User::getID: Be lazier when fetching UserNameUtils

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

So the trigger for this problem is probably >https://gerrit.wikimedia.org/r/c/mediawiki/core/+/683726/14/includes/session/SessionManager.php#229>, which causes SessionManager to ask the service container for a UserNameUtils object in the constructor. We could do that elsewhere to fix the problem for now. But at some point, we'll want to convert to proper DI, and we'll have the same chicke-and-egg issue agin.

I still don't see how we are accessing a SessionManager from the wiring of WANObjectCache anyway.
I made https://gerrit.wikimedia.org/r/c/mediawiki/core/+/688264 to force the Session constructor to throw when called from inside WANObjectCahe's wiring, assuming that, if the destructor is called from that scope, the constructor was also called from that scope somewhow, and we'd get a stack trace. But I can't make it trigger....

UserNameUtils is loaded in User::getID, which is called in the SessionBackend destructors (it's one of the few calls that are made even if no session operation happened). https://gerrit.wikimedia.org/r/c/mediawiki/core/+/688308 works around that (at least for anonymous users, but there shouldn't be any other kind of user during setup). It does fix the test error, which would imply this was caused by https://gerrit.wikimedia.org/r/c/mediawiki/core/+/678604 but the timing does not line up...

In any case, as you say, the bigger mystery is why sessions are involved at all at this point. Something in that ObjectCache logic must have created and disposed a SessionBackend object, but I have no idea what that is. Nor how it could be related to GrowthExperimentsMultiConfig (presumably the reason why this error only shows up in GrowthExtensions) or why MultiLock is the single special page for which the test fails. It does depend on UserNameUtils, but it's not the only special page that does. And in any case the loading of that service wasn't triggered via that dependency.

Change 688308 merged by jenkins-bot:

[mediawiki/core@master] User::getID: Be lazier when fetching UserNameUtils

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

Change 687415 abandoned by Gergő Tisza:

[mediawiki/extensions/GrowthExperiments@master] [DNM] CI test

Reason:

not needed anymore

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

The GrowthExperiments CI error is fixed (PHPUnit tests never have a valid session so limiting the UserNameUtils dependency to non-anonymous users will prevent the error from occuring in tests). But the underlying issue of the WANObjectCache service factory creating a session for no obvious reason is not fixed, and seems like the kind of thing that can cause trouble in the future.

Krinkle changed the task status from Declined to Resolved.May 11 2021, 1:42 AM