Page MenuHomePhabricator

Introduce ThrottleStore
Closed, ResolvedPublic

Description

Reduce coupling by introducing a ThrottleStore service, for use by User::pingLimiter and Auth\Throttler. This would allow throttling behavior of different components to be tested easily, and would centralize the implementation of throttling algorithms.

Draft:

class ThrottleStore {

    /**
     * @param string $realm Supports at least "local" or "global"
     * @param string $key 
     * @param array[] conditions A list of conditions. Each condition is an associative array 
     *                with the following keys: 'limit' is the maximum count per window, and
     *                'window' is the duration of a window in seconds.   
     * @param int $amount the amount of the throttled resource to consume. When throttling
     *                occurrences, this would be 1. When throttling volume, this would be the size the
     *                current chunk. For peeking at the throttle without changing it, use zero.
     * @return array[] A list of throttle states, with each state corresponding to a condition,
     *                and having the following keys: 'count' the current count against the limit,
     *                'wait' the number of seconds to with before the next attempt. The throttle
     *                is "triggered" if at least one 'wait' is larger than 0.
     **/
    public function throttle( string $realm, string $key, array $conditions, $amount  );
}

In order to abstract different throttling algorithms, we could use the strategy pattern, allowing the ThrottleStore to use strategies like fixed window or leaky bucket.

This interface shouldn't be used directly by application logic. We'd want to wrap this in a more high level abstraction that offers methods like throttleLogin( $name, $ip ) or throttleEdit( $session ), etc.

Related Objects

StatusSubtypeAssignedTask
ResolvedNone
OpenNone
OpenNone
Resolved Pchelolo
Resolved Pchelolo
Resolved Pchelolo
Resolved Pchelolo
Resolved Pchelolo
Resolved CCicalese_WMF
OpenNone
OpenNone
DuplicateNone
InvalidNone
Resolved Pchelolo
Resolved CCicalese_WMF
OpenNone
Resolvedmatmarex
ResolvedNone
Resolveddaniel
OpenNone
Resolved Pchelolo
Resolved Pchelolo
ResolvedNone
OpenNone
Resolved CCicalese_WMF
Resolveddaniel
OpenNone
OpenNone
Resolveddaniel

Event Timeline

+1 Maybe ThrottleEngine? We use Store for low-level DB access stuff with CRUD operations, this is very different from a Store in MW.

Change 657224 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/core@master] WIP: Bikeshed on how to organize a throttler for authority.

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

I'm not keen on "throttle" as jargon for rate limiting. It has violent connotations when applied to human actions. Throttling an engine sounds OK but throttling a vandal seems questionable.

RateLimiter is the obvious choice, and according to code search it is not used too much in MediaWiki. But I am amusing myself by browsing the thesaurus, which has words like "fetter" and "retard", the latter being a verb -- a thing that retards would be a retardance.

Change 807512 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/core@master] [WIP] Stats library WRStats

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

Change 808201 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/core@master] [WIP] Add rate limiter support to WRStats

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

Change 810322 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/core@master] Make RateLimiter use WRStats

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

Change 810839 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/core@master] Optimised SqlBagOStuff::incrWithInit with WRITE_BACKGROUND

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

Change 810839 merged by jenkins-bot:

[mediawiki/core@master] objectcache: Optimise SqlBagOStuff::incrWithInit with WRITE_BACKGROUND

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

Change 808201 merged by jenkins-bot:

[mediawiki/core@master] WRStats: Add rate limiter support

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

Change 810322 merged by jenkins-bot:

[mediawiki/core@master] Make RateLimiter use WRStats

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

This task is done or obsolete, right? pingLimiter uses WRStatsRateLimiter, which provides an abstraction layer for the throttle state.

Throttler should also switch to using WRStats, but there's a separate task for that.

daniel claimed this task.

This task is done or obsolete, right? pingLimiter uses WRStatsRateLimiter, which provides an abstraction layer for the throttle state.

Throttler should also switch to using WRStats, but there's a separate task for that.

Yea I think you are right

Change #657224 abandoned by Hashar:

[mediawiki/core@master] WIP: Bikeshed on how to organize a throttler for authority.

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

Change #657224 restored by Thcipriani:

[mediawiki/core@master] WIP: Bikeshed on how to organize a throttler for authority.

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