Page MenuHomePhabricator

TitleLocalCache can break tests because it's never reset
Closed, ResolvedPublic

Description

TitleLocalCache is a singleton class that maps page IDs to Title objects. This cache is never invalidated, which means that if a page is deleted, the data it stores will become stale. This is especially a problem in PHPUnit tests, where any pages are (or at least should be) deleted after each test that creates them, and where page IDs are not stable because we use a test database (i.e., IDs always restart from 1).

These issues can be observed in core change r946549, which keeps failing because some extension tests are not cleaning up the DB, and therefore TitleLocalCache stores stale data.

Potential solutions to this:

  • Reset the singleton after each test. This might be easy to implement, but it's not the direction we should be heading to. Especially because there might be other singleton classes with similar issues, and core should really not be aware of any of them.
  • Make the class not be a singleton. Probably not possible to do easily in a useful way, because TitleLocalCache is used by Event, which is usually created via a static factory method.
  • Make TitleLocalCache a proper service (and not a singleton). This would solve our issues because all services are reset after tests.
  • And finally: get rid of this class. I don't know how easy this would be, but I believe that if retrieving all those titles is expensive, then the cache should be in PageStore or another core class, and not in extension code.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Oh, and of course, I would expect RevisionLocalCache to have similar issues, though I've never observed those in practice thus far.

I'm not sure if the last option is doable, because I'm not familiar with Echo (and its performance implications) and PageStore. Adding some tags to give this visibility.

Change 948610 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/extensions/Echo@master] Make Title and Revision caches proper services

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

Patch above makes both classes real services. I'm still convinced they shouldn't exist at all, but for now this seems the easiest thing to do.

Change 948610 merged by jenkins-bot:

[mediawiki/extensions/Echo@master] Make Title and Revision caches proper services

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

I'm going to close this because the immediate issue has been resolved.