Page MenuHomePhabricator

Can't correctly unserialize cached EntityRevision on WikibaseClient (Wikipedia)
Open, HighPublic

Description

In a case when WikibaseRepo (wikidata) has Lexeme extension enabled, and WikibaseClient (wikipedia) does not, and they share the object cache where EntityRevision objects are stored, on the read on the Client side fatal errors happen due to unserializing unknown class (LexemeId in this case)

See log bit from test.wikipedia:

Catchable fatal error: Argument 1 passed to Wikibase\Client\Usage\UsageTrackingSnakFormatter::addLabelUsage() must be an instance of Wikibase\DataModel\Entity\EntityId, __PHP_Incomplete_Class given in...

Repo and Client share cache prefixes, thus whatever Repo puts in the cache, is read by Client. In case both systems run different version of software (e.g. one has WikibaseLexeme enabled, and the other not), Client code is bound to deal with unknown code.

Related to: T195615.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 14 2018, 5:26 PM

Some options to overcome the issue (list not complete and not sorted in any way)

  1. Always run the same version of the software on both systems.

Tricky/Maybe even not wanted, as not only means enabling WikibaseLexeme, WikibaseMediaInfo etc on wikiedias, but to get things 100% right, means always having the same version of Wikibase running on both systems. Not doable with the current deployment strategy with deploying Wikibase to different wikis on different days

  1. Do not share cache prefixes between Repo and Client unless they're the same wiki.

Sharing cache between repo and client makes sense in the single-wiki case (e.g. wikidata.org), but is this really needed otherwise?
Little bit of random archeology and finding https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/Wikibase/+/203811/ does not really provide the answer why the shared cache prefix has been introduced.

  1. Handle unserialization of unknown classes more "gracefully", using http://php.net/manual/en/var.configuration.php#ini.unserialize-callback-func.

We could make said callback throw an exception that Wikibase could handle (e.g. ignore reading from cache in such case). Problem is the setting is global, and could lead to all sort of unforeseen issues.

Pinging @hoo @Amir1 @aude @daniel @ArielGlenn as they might have some clue here, or know some background.

Aklapper renamed this task from Can't correclty unserialize cached EntityRevision on WikibaseClient (Wikipedia) to Can't correctly unserialize cached EntityRevision on WikibaseClient (Wikipedia).Jun 14 2018, 5:27 PM
Aklapper added a project: Wikibase-Lua.
Restricted Application added a project: Wikidata. · View Herald TranscriptJun 14 2018, 5:27 PM
Addshore added a comment.EditedJun 15 2018, 7:42 AM

https://github.com/wikimedia/operations-mediawiki-config/blob/master/wmf-config/Wikibase.php#L37-L48

Some options to overcome the issue (list not complete and not sorted in any way)

  1. Always run the same version of the software on both systems.

Tricky/Maybe even not wanted, as not only means enabling WikibaseLexeme, WikibaseMediaInfo etc on wikiedias, but to get things 100% right, means always having the same version of Wikibase running on both systems. Not doable with the current deployment strategy with deploying Wikibase to different wikis on different days

I do not like this idea.
This means when rolling out new extensions we just have to flip one almighty switch and load it everywhere at once, sounds scary, especially as we will have a rollout like lexeme, but for media info again in the not to distant future.

  1. Do not share cache prefixes between Repo and Client unless they're the same wiki.

Sharing cache between repo and client makes sense in the single-wiki case (e.g. wikidata.org), but is this really needed otherwise?
Little bit of random archeology and finding https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/Wikibase/+/203811/ does not really provide the answer why the shared cache prefix has been introduced.

So the cache key for production is created here: https://github.com/wikimedia/operations-mediawiki-config/blob/master/wmf-config/Wikibase.php#L37-L48
It currently only varies on the branch of mediawiki & wikibase that is deployed and also the repo name (testwikidata vs real wikidata)
Right now we have some different combinations that this doesn't account for

  • Wikibase & No Lexeme
  • Wikibase & Lexeme

During the the MediaInfo rollout this will increase again, I'm not sure what the final list of clients for which repos etc will look like or how it will roll out at all.

One important thing to find out here is how much cache space do we actually use?
Is having more keys a viable possibility?
Having different keys based on the deployed wikibase extensions seems feasible to me

  1. Handle unserialization of unknown classes more "gracefully", using http://php.net/manual/en/var.configuration.php#ini.unserialize-callback-func.

We could make said callback throw an exception that Wikibase could handle (e.g. ignore reading from cache in such case). Problem is the setting is global, and could lead to all sort of unforeseen issues.
Pinging @hoo @Amir1 @aude @daniel @ArielGlenn as they might have some clue here, or know some background.

What line in Wikibase actually does this unserialization? perhaps we could change that to something that wouldn't have such a global effect.

Pinging @Ladsgroup because he was missed by Phabricator

oh I messed up pings.
E.g. I wanted to ping @aaron not Ariel. Sorry folks!

What line in Wikibase actually does this unserialization? perhaps we could change that to something that wouldn't have such a global effect.

That's where the fun lies. Technically speaking, Wikibase itself does not do serialize/unserialize. PHP serialization happens in EntityRevisionCache::set/get which are basically calling set/get on BagOStuff putting EntityRevision instance in the cache.

That actually brings up another option we've briefly mentioned in the exploration group yesterday (as with others, I am not suggesting it being a good pick):

  1. Do not store EntityRevision objects into MW cache, but use own serialization of those objects or so.

Do problem of handling of unknown EntityIds etc would not really be solved. Wikibase would be more in the control of such situations, though. Which might be good, or not that good thing.

What line in Wikibase actually does this unserialization? perhaps we could change that to something that wouldn't have such a global effect.

That's where the fun lies. Technically speaking, Wikibase itself does not do serialize/unserialize. PHP serialization happens in EntityRevisionCache::set/get which are basically calling set/get on BagOStuff putting EntityRevision instance in the cache.
That actually brings up another option we've briefly mentioned in the exploration group yesterday (as with others, I am not suggesting it being a good pick):

  1. Do not store EntityRevision objects into MW cache, but use own serialization of those objects or so.

Do problem of handling of unknown EntityIds etc would not really be solved. Wikibase would be more in the control of such situations, though. Which might be good, or not that good thing.

Yup, we could just put our JSON there instead of serializing a php object (which just sucks 99% of the time anyway), then we could handle deserialization issues in a local way, rather than the global approach of unserialize-callback-func.

There might be some difference in memory consumption or or time to deserialize to consider, but probably nothing too bad.

hoo added a comment.Jun 15 2018, 1:35 PM

There might be some difference in memory consumption or or time to deserialize to consider, but probably nothing too bad.

Well, I vaguely remember loading the php-serialization from cache being about 30% faster than deserializing JSON, but I can't quite remember where I benchmarked this (probably benchmarked this while working on dumps). When considering this, we should really benchmark this thoroughly.

My preferred option would be to make the fetching from the cache more resilient against instances of unknown classes in there… maybe using at-ease?

Vvjjkkii renamed this task from Can't correctly unserialize cached EntityRevision on WikibaseClient (Wikipedia) to 3zaaaaaaaa.Jul 1 2018, 1:03 AM
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed a subscriber: Aklapper.
ArielGlenn renamed this task from 3zaaaaaaaa to Can't correctly unserialize cached EntityRevision on WikibaseClient (Wikipedia) .Jul 1 2018, 6:10 AM
ArielGlenn updated the task description. (Show Details)
ArielGlenn added a subscriber: Aklapper.

Well, I vaguely remember loading the php-serialization from cache being about 30% faster than deserializing JSON, but I can't quite remember where I benchmarked this (probably benchmarked this while working on dumps). When considering this, we should really benchmark this thoroughly.

@hoo any chance you'd be able to dig up this benchmarking? Not necessarily the exact results, but maybe when it happened, and in which context it has been measured?
Asking, because I'd be a bit reluctant to give up on something that seems "a right thing" if we're not completely sure of what performance implication are NOW.