Page MenuHomePhabricator

Make RESTBagOStuff::incr() atomic
Open, Needs TriagePublic

Description

from https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/core/+/master/includes/libs/objectcache/RESTBagOStuff.php#150

per @aaron

It currently does a get() before set(), which I think has race conditions? It's not necessary for session storage, though.

Event Timeline

So, this isn't required for session storage. I'm going re-tag it, but I don't think it should block.

daniel added a subscriber: daniel.EditedMay 28 2019, 2:17 PM

While having an atomic incr() method would certainly be nice, this task doesn't make sense to me for two reasons:

  1. The BagOStuff interface doesn't require or even suggest incr() to be atomic, and non-atomic implementations exist (e.g. in RedisBagOStuff). Because of this, there should be no callers that require atomic semantics.
  2. RESTBagOStuff is designed to work with any RESTish CRUD service. There's a variety of ways to implement atomic increment on top of such an interface, but all of them require some level of cooperation from the backend, and all of them come with cost in terms of latency or failure modes. It's unclear which mechanism RESTBagOStuff should use, this would have to be left to a subclass.

Overall, BagOStuff is overloaded and under-specified. We should introduce new, more narrow interfaces that provide more guarantees about the semantics of the methods they define. An interface that is designed to fit any key-value-store will lack the guarantees that would make it useful in any but the most trivial use cases.

aaron added a comment.May 28 2019, 9:08 PM

While having an atomic incr() method would certainly be nice, this task doesn't make sense to me for two reasons:

  1. The BagOStuff interface doesn't require or even suggest incr() to be atomic, and non-atomic implementations exist (e.g. in RedisBagOStuff). Because of this, there should be no callers that require atomic semantics.
  2. RESTBagOStuff is designed to work with any RESTish CRUD service. There's a variety of ways to implement atomic increment on top of such an interface, but all of them require some level of cooperation from the backend, and all of them come with cost in terms of latency or failure modes. It's unclear which mechanism RESTBagOStuff should use, this would have to be left to a subclass.

Overall, BagOStuff is overloaded and under-specified. We should introduce new, more narrow interfaces that provide more guarantees about the semantics of the methods they define. An interface that is designed to fit any key-value-store will lack the guarantees that would make it useful in any but the most trivial use cases.

Anything that can be atomic should be, or it gets kind of useless, for BagOStuff. The redis issue is a bug and should be fixed (infinite TTL stuff can get stuck). If a backend can't do incr() right, then it should implement a simpler interface. I agree about extra interfaces or something to narrow down what kind of operations a backend can easily do. That's part of why I made WANObjectCache it's own layer (among other reasons like cache-specific semantics and related useful assumptions). I keep thinking about migrating the main stash to a simpler interface, though I've never got around to it.