Page MenuHomePhabricator

ResourceLoader should not hide circular dependencies server-side
Closed, ResolvedPublic

Description

We currently detect circular dependencies on the client-side only, not the server-side. On its own that seems fine. We need it on the client either way, and keeping the server side simpler has its benefits.

What matters is that CI detects the problem, and that in production we can find the problem with a descriptive error message. Client-side can and does do that.

But, only in theory. In practice, right now, this logic is never triggered because on the server-side we do something that causes circular dependencies to be silently removed. Thus, we hear nothing server-side, and hear nothing client-side.

Instead, the module executes as if it doesn't have the branch of dependencies that contains the circular dependency. Oops!

Cause

We hold as best practice for modules to declare their own needs, regardless of whether an indirect dependency has the same need. For example, if a module reads cookies via mw.cookie, it should declare a dependency on mediawiki.cookie. Developers are encouraged to do this, even if A depends on B which also reads cookies internally.

As optimisation for transfer size (bandwidth) and memory (client-side), we shake out any dependencies that are redundant given the current tree. So in this case, it would transfer A > B > mediawiki.cookie. Ultimately what matters is that needing "A" results in "A + B + mediawiki.cookie" being downloaded, and executed in order "mediawiki.cookie > B > A". The tree is merely a tool to make that happen, and can thus safely be optimised in this way.

But, the logic currently has a bug that is causing modules with a circular dependency to be exported as having no dependencies at all.

Event Timeline

Krinkle created this task.May 15 2019, 6:21 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 15 2019, 6:22 PM
Krinkle triaged this task as High priority.May 15 2019, 6:22 PM
Gilles assigned this task to Krinkle.May 20 2019, 8:20 PM

Change 511941 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] resourceloader: Add tests for StartUpModule dep tree optimisation

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

Change 511942 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] [WIP] resourceloader: Support circular deps in tree optimiser

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

Change 511941 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Add tests for StartUpModule dep tree optimisation

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

Change 511942 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Skip modules with circular deps in tree optimiser

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

Krinkle closed this task as Resolved.Jun 18 2019, 11:07 PM
Krinkle removed a project: Patch-For-Review.