Page MenuHomePhabricator

ResourceLoader should not fatal the site due to duplicate module registration
Closed, ResolvedPublic

Description

An incident (https://wikitech.wikimedia.org/w/index.php?title=Incident_documentation/20151026-MediaWiki) occurred due to multiple registration of a module after the patches were deployed in the wrong order. This should log an error and continue working as best as possible rather than taking down the entire site.

MWException from line 331 of /srv/mediawiki/php-1.27.0-wmf.3/includes/resourceloader/ResourceLoader.php: ResourceLoader duplicate registration error. Another module has already been registered as schema.Search

Details

Related Gerrit Patches:

Event Timeline

EBernhardson raised the priority of this task from to Needs Triage.
EBernhardson updated the task description. (Show Details)
EBernhardson added a subscriber: EBernhardson.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 26 2015, 6:11 PM

Change 248932 had a related patch set uploaded (by Ori.livneh):
Deduplicate identical module registrations

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

Change 248932 merged by jenkins-bot:
resourceloader: Deduplicate module registration conflicts

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

Vituzzu added a subscriber: Vituzzu.

ResourceLoader explicitly did not allow this because modules cannot be uniquely identified by name, no more than a PHP class can be. Implementation can vary regardless of the name. And more often than not, duplicate registrations are not identical. Perhaps even incompatible.

Defining the same PHP class in two deployed repositories would also result in a fatal error (though it might be masked by our autoloader, since we lazy-load class definition files).

I've merged the change to emit a warning instead of fatal error (https://gerrit.wikimedia.org/r/248932) which should suffice in practice (so long we monitor warnings during deployment). This will mitigate outages because front-end run-time degrades more gracefully.

However for the record: The correct order to deploy such changes is not relevant, as much as those changes should not be deployed in separate syncs in the first place. Apply the patches on tin and sync in one broader sync-dir (or scap, or wait for the train, or not at all).

The loosening of this warning does not change that such changes must be deployed in one sync to avoid race conditions, and reduce Varnish cache poisoning.

The "Structure" test suite in MediaWiki detects registration conflicts. However the patches were merged in the correct order. Note that as of https://gerrit.wikimedia.org/r/248932, this suite will no longer catch this problem during development, so we have to fill that back. We don't want to wait until production to discover unintended name conflicts. This has caused problems in the past when wikidata, fundraising and another extension had conflicting versions of a jQuery plugin – which unfolded differently in production due to extension load order.

MaxSem added a subscriber: MaxSem.Oct 27 2015, 7:40 AM

We can actually throw exceptions in developer mode to increase visibility, otherwise a PHP warning would seem like a safer option.

Krinkle closed this task as Resolved.Nov 12 2015, 10:42 PM
Krinkle claimed this task.

Per b1ea0612f8beec5af1142da5b8075ab3427efa4c and d3e3bcfd6daad1f2a5894129b7e6d2f5c4390a6f, there is now a warning logged to the resourceloader channel. Will show up in the debug log by default.

Restricted Application added a subscriber: StudiesWorld. · View Herald TranscriptNov 12 2015, 10:42 PM
Krinkle set Security to None.
Krinkle removed a project: Patch-For-Review.