Page MenuHomePhabricator

ServiceContainer.php: Circular dependency when creating MobileFrontend service "AMC.UserMode > AMC.Manager > FeaturesManager > UserModes > AMC.UserMode"
Closed, ResolvedPublic

Description

Error

Request ID: XV6kbApAADgAACft8eEAAACN
Request URL: en.wikipedia.org/wiki/Special:History/Grampian,_Highland_and_Islands%3Faction%3Dhistory

message
[XV6kbApAADgAACft8eEAAACN] /wiki/Special:History/Grampian,_Highland_and_Islands%3Faction%3Dhistory   RuntimeException from line 449 of /srv/mediawiki/php-1.34.0-wmf.19/includes/libs/services/ServiceContainer.php: Circular dependency when creating service! MobileFrontend.AMC.UserMode -> MobileFrontend.AMC.Manager -> MobileFrontend.FeaturesManager -> MobileFrontend.UserModes -> MobileFrontend.AMC.UserMode
trace
#0 /srv/mediawiki/php-1.34.0-wmf.19/includes/libs/services/ServiceContainer.php(427): Wikimedia\Services\ServiceContainer->createService(string)
#1 /srv/mediawiki/php-1.34.0-wmf.19/extensions/MobileFrontend/includes/ServiceWiring.php(20): Wikimedia\Services\ServiceContainer->getService(string)
#2 /srv/mediawiki/php-1.34.0-wmf.19/includes/libs/services/ServiceContainer.php(459): Closure$#2(MediaWiki\MediaWikiServices)
#3 /srv/mediawiki/php-1.34.0-wmf.19/includes/libs/services/ServiceContainer.php(427): Wikimedia\Services\ServiceContainer->createService(string)
#4 /srv/mediawiki/php-1.34.0-wmf.19/extensions/MobileFrontend/includes/ServiceWiring.php(25): Wikimedia\Services\ServiceContainer->getService(string)
#5 /srv/mediawiki/php-1.34.0-wmf.19/includes/libs/services/ServiceContainer.php(459): Closure$#3(MediaWiki\MediaWikiServices)
#6 /srv/mediawiki/php-1.34.0-wmf.19/includes/libs/services/ServiceContainer.php(427): Wikimedia\Services\ServiceContainer->createService(string)
#7 /srv/mediawiki/php-1.34.0-wmf.19/extensions/MobileFrontend/includes/specials/SpecialMobileHistory.php(94): Wikimedia\Services\ServiceContainer->getService(string)
#8 /srv/mediawiki/php-1.34.0-wmf.19/extensions/MobileFrontend/includes/MobileContext.php(364): SpecialMobileHistory::shouldUseSpecialHistory(Title, User)
#9 /srv/mediawiki/php-1.34.0-wmf.19/extensions/MobileFrontend/includes/MobileContext.php(339): MobileContext->redirectMobileEnabledPages()
#10 /srv/mediawiki/php-1.34.0-wmf.19/extensions/MobileFrontend/includes/ServiceWiring.php(48): MobileContext->shouldDisplayMobileView()
#11 /srv/mediawiki/php-1.34.0-wmf.19/includes/libs/services/ServiceContainer.php(459): Closure$#4(MediaWiki\MediaWikiServices)
#12 /srv/mediawiki/php-1.34.0-wmf.19/includes/libs/services/ServiceContainer.php(427): Wikimedia\Services\ServiceContainer->createService(string)
#13 /srv/mediawiki/php-1.34.0-wmf.19/extensions/MobileFrontend/includes/ServiceWiring.php(54): Wikimedia\Services\ServiceContainer->getService(string)
#14 /srv/mediawiki/php-1.34.0-wmf.19/includes/libs/services/ServiceContainer.php(459): Closure$#5(MediaWiki\MediaWikiServices)
#15 /srv/mediawiki/php-1.34.0-wmf.19/includes/libs/services/ServiceContainer.php(427): Wikimedia\Services\ServiceContainer->createService(string)
#16 /srv/mediawiki/php-1.34.0-wmf.19/extensions/MobileFrontend/includes/MobileFrontendHooks.php(476): Wikimedia\Services\ServiceContainer->getService(string)
#17 /srv/mediawiki/php-1.34.0-wmf.19/includes/Hooks.php(174): MobileFrontendHooks::onSpecialPageInitList(array)
#18 /srv/mediawiki/php-1.34.0-wmf.19/includes/Hooks.php(202): Hooks::callHook(string, array, array, NULL)
#19 /srv/mediawiki/php-1.34.0-wmf.19/includes/specialpage/SpecialPageFactory.php(304): Hooks::run(string, array)
#20 /srv/mediawiki/php-1.34.0-wmf.19/includes/specialpage/SpecialPageFactory.php(320): MediaWiki\Special\SpecialPageFactory->getPageList()
#21 /srv/mediawiki/php-1.34.0-wmf.19/includes/specialpage/SpecialPageFactory.php(378): MediaWiki\Special\SpecialPageFactory->getAliasList()
#22 /srv/mediawiki/php-1.34.0-wmf.19/includes/Title.php(1204): MediaWiki\Special\SpecialPageFactory->resolveAlias(string)
#23 /srv/mediawiki/php-1.34.0-wmf.19/includes/MediaWiki.php(190): Title->isSpecial(string)
#24 /srv/mediawiki/php-1.34.0-wmf.19/includes/MediaWiki.php(892): MediaWiki->performRequest()
#25 /srv/mediawiki/php-1.34.0-wmf.19/includes/MediaWiki.php(523): MediaWiki->main()
#26 /srv/mediawiki/php-1.34.0-wmf.19/index.php(42): MediaWiki->run()
#27 /srv/mediawiki/w/index.php(3): include(string)
#28 {main}
Impact

13 hits in the last hour, 19 in the last 24 hours. First seen on 2019-08-20T21:51:12.
445 hits / hour as of 2019-08-26T18:42:41.

Notes

Since it doesn't happen a lot, I'm not reverting this week's train, but making this a blocker for next week.

https://logstash.wikimedia.org/goto/ead42b1837ac196767f0bf50498990fb

QA Steps

Things to test:

a) go to /wiki/Special:History/{test_article}?action=history (where {test_article} is the article of your choosing. The page should render correctly.
b) opt in/opt out into AMC mode
c) disable/enable AMC mode via config,

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptThu, Aug 22, 2:38 PM
zeljkofilipin triaged this task as Unbreak Now! priority.Thu, Aug 22, 2:39 PM
Restricted Application added a subscriber: Liuxinyu970226. · View Herald TranscriptThu, Aug 22, 2:40 PM

That's pretty weird, as a first glympse, I don't see where MobileFrontend.AMC.Manager depends on MobileFrontend.FeaturesManager

Krinkle renamed this task from ServiceContainer.php: Circular dependency when creating service! MobileFrontend.AMC.UserMode -> MobileFrontend.AMC.Manager -> Mobi to ServiceContainer.php: Circular dependency when creating MobileFrontend service "AMC.UserMode > AMC.Manager > FeaturesManager > UserModes > AMC.UserMode".Thu, Aug 22, 3:18 PM

The question is, did this work before, or crash with a stack overflow? Do we log all fatal errors in such a way that we know these page views actually succeeded before this change, or might the error just have changed?

I did consider when writing the check that it's possible for recursion to be happening in a way that terminates, in which case the check would break otherwise working pages, but it seemed unlikely.

That's pretty weird, as a first glympse, I don't see where MobileFrontend.AMC.Manager depends on MobileFrontend.FeaturesManager

Did you look at the stack trace?

#6 /srv/mediawiki/php-1.34.0-wmf.19/includes/libs/services/ServiceContainer.php(427): Wikimedia\Services\ServiceContainer->createService(string)
#7 /srv/mediawiki/php-1.34.0-wmf.19/extensions/MobileFrontend/includes/specials/SpecialMobileHistory.php(94): Wikimedia\Services\ServiceContainer->getService(string)
#8 /srv/mediawiki/php-1.34.0-wmf.19/extensions/MobileFrontend/includes/MobileContext.php(364): SpecialMobileHistory::shouldUseSpecialHistory(Title, User)
#9 /srv/mediawiki/php-1.34.0-wmf.19/extensions/MobileFrontend/includes/MobileContext.php(339): MobileContext->redirectMobileEnabledPages()
#10 /srv/mediawiki/php-1.34.0-wmf.19/extensions/MobileFrontend/includes/ServiceWiring.php(48): MobileContext->shouldDisplayMobileView()
#11 /srv/mediawiki/php-1.34.0-wmf.19/includes/libs/services/ServiceContainer.php(459): Closure$#4(MediaWiki\MediaWikiServices)

MobileFrontend.AMC.Manager's instantiation function calls MobileFrontend.Context's shouldDisplayMobileView() method. That method, if $this->shouldDisplayMobileViewInternal() returns true, calls $this->redirectMobileEnabledPages(). If the request is for action=history and the title is not null, this then calls SpecialMobileHistory::shouldUseSpecialHistory(). That then tries to instantiate MobileFrontend.FeaturesManager.

So this looks like it will occur on any attempt to view a history page on mobile. But it also looks like it would previously have worked despite the circular dependency. When it tried to instantiate MobileFrontend.AMC.Manager the second time, shouldDisplayMobileView() would short-circuit because $this->mobileView is already set. This would allow all the necessary services to be created just fine, although I don't know if it would result in any bugs.

In short, it looks like the code would previously have worked, but the current behavior is probably not intended and it's good that we caught it. I suggest:

  1. We add a bit of tolerance to the recursion check, and allow it to continue to some reasonable stack depth before terminating it even if we find a circular dependency. A circular dependency should log a warning in any event. I'll work on that now.
  1. If desired, the commit a995d9be1df can be reverted for now. There should be no conflicts or likelihood of bad stuff happening, the change is simple and self-contained and doesn't affect normal operation.
  1. The developers of MobileFrontend might want to consider if their code is correct here. (If SpecialMobileHistory was an instance method of something that got its services injected instead of a static method that used MediaWikiServices, the circularity would have been immediately obvious.)

Change 531729 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/core@master] Warn on circular service dependencies, don't throw

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

pmiazga added a comment.EditedThu, Aug 22, 6:26 PM

@Simetrical thx for the comment, yeah, you're right, I missed that SpecialMobileHistory::shouldUseSpecialHistory gets the FeatureManager. I only focused on ServiceWiring and AMC codebase. I'll fix that code. circular dependencies are bad and such situation should not have place.

Change 531729 abandoned by simetrical:
Warn on circular service dependencies, don't throw

Reason:
If we want it to be fatal, I don't have a problem with that.

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

Change 532389 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/MobileFrontend@master] Postpone call to MobileContext::shouldDisplayMobileView()

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

pmiazga reassigned this task from pmiazga to Edtadros.Mon, Aug 26, 5:11 PM
pmiazga added a subscriber: pmiazga.
Niedzielski added a subscriber: Edtadros.
pmiazga updated the task description. (Show Details)Mon, Aug 26, 5:20 PM

Friendly reminder, this is blocking the train. Will it be resolved by train window tomorrow?

@zeljkofilipin yes, the fix got merged like 2 hours ago, but the CI is still processing it ;/

Niedzielski removed Niedzielski as the assignee of this task.Mon, Aug 26, 6:48 PM
Niedzielski added a subscriber: Niedzielski.

Visit http://localhost:8181/wiki/Special:History/Foo?action=history on a mobile device.

@pmiazga and I are no longer able to reproduce this issue locally with or without the patch.

Change 532389 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Postpone call to MobileContext::shouldDisplayMobileView()

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

@zeljkofilipin this should be fixed now, can we resolve task once this goes to production? We're unable to reproduce it locally.

@pmiazga if you think it's fixed, resolve the task. I'll reopen it if I see the error in the logs after the deployment.

ok, thx for info

Reedy reopened this task as Open.Wed, Aug 28, 4:33 PM
Reedy added a subscriber: Reedy.

Seems to still be an issue

Seeing this again in production on enwiki as of about 15:15 UTC today https://logstash.wikimedia.org/goto/971e0dc9c8f3d9cfe4d76c30a6446a9b

Change 533064 had a related patch set uploaded (by Jforrester; owner: Pmiazga):
[mediawiki/extensions/MobileFrontend@wmf/1.34.0-wmf.19] Postpone call to MobileContext::shouldDisplayMobileView()

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

Change 533064 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@wmf/1.34.0-wmf.19] Postpone call to MobileContext::shouldDisplayMobileView()

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

Mentioned in SAL (#wikimedia-operations) [2019-08-28T17:06:14Z] <jforrester@deploy1001> Synchronized php-1.34.0-wmf.19/extensions/MobileFrontend/includes: T231014 Postpone call to MobileContext::shouldDisplayMobileView() (duration: 00m 55s)

Error rate back to nominal.

Jdforrester-WMF closed this task as Resolved.Wed, Aug 28, 5:18 PM
Jdforrester-WMF claimed this task.
mmodell changed the subtype of this task from "Task" to "Production Error".Wed, Aug 28, 11:05 PM