Page MenuHomePhabricator

API action=parse should be poolcounter-limited if a re-parse is necessary
Closed, ResolvedPublic

Description

When some parameter in a request for action=parse makes it bypass the parser cache, or more in general if a full reparse is needed, we should limit such actions per user (or IP) to N (with possibly N=3 ?) concurrent executions, using poolcounter.

We had a bot today making about 100 rps for pages like

http://zh.wikipedia.org/w/api.php?action=parse&pageid=2868367&prop=text&wrapoutputclass=wiki-article&disableeditsection=true&mobileformat=true&mainpage=true&format=json

and that caused a decided slowdown of the API cluster. We need to limit such users not to consume a significant portion of our computing power.

Event Timeline

Joe created this task.Jan 27 2020, 11:41 PM
Restricted Application added subscribers: Cosine02, Aklapper. · View Herald TranscriptJan 27 2020, 11:41 PM
Joe triaged this task as High priority.Jan 27 2020, 11:41 PM
Krinkle moved this task from On-going to Follow-up prevention on the Wikimedia-Incident board.
jbond added a subscriber: jbond.Feb 7 2020, 3:09 PM
AMooney removed nnikkhoui as the assignee of this task.Apr 1 2020, 6:48 PM
AMooney added a subscriber: nnikkhoui.
AMooney added a subscriber: AMooney.

Peter, Can you take a look at this? If needed work with Brad :)

Talked with @Anomie and he brought up a good point that it may be worth it to implement PoolCounter for all requests, regardless of whether or not it bypasses ParserCache. That would lead to less complicated/case-by-case code and even if the request does hit ParserCache it may still be a cache miss and it would have to do the full slower parse anyway.

@Anomie, @nnikkhoui,

Do I understand correctly that PoolWorkArticleView (inherited from PoolCounterWork) is the main point responsible for pooling all king of requests? So there's no way to bypass it.

	public function execute( $skipcache = false ) {
		if ( $this->cacheable && !$skipcache ) {
			$status = $this->poolCounter->acquireForAnyone();
		} else {
			$status = $this->poolCounter->acquireForMe();
		}

And then is should be possible to verride nested counter to make it more strick for User/IP addresses?
Do I understand this structure correctly?

Anomie added a comment.Apr 3 2020, 4:02 PM

PoolWorkArticleView is specifically for article views, that probably wouldn't quite work for this code path as it also needs to parse arbitrary wikitext.

It would probably be better to use PoolCounterWorkViaCallback than to try to use PoolCounter directly, as in the latter case you'd probably have to duplicate all the logic from PoolCounterWork::execute() anyway.

You don't need to worry about handling the concurrency limits yourself, PoolCounter does that for you as long as you set up your keys properly. In general, your code just needs to look something like this:

$work = new PoolCounterWorkViaCallback( 'Key-for-$wgPoolCounterConf', $poolKey, [
    'doWork' => function () use ( $whatever ) {
        /* Expensive code to parse things and generate $p_result */
        return $p_result;
    },
    'error' => function () {
        $this->dieWithError( 'apierror-concurrency-limit' );
    },
] );
$p_result = $work->execute();

The 'Key-for-$wgPoolCounterConf' is just that, the key in $wgPoolCounterConf that specifies the limits to apply. Those would get set in Wikimedia's configuration, something like rOMWC8274bc4f3b29: Add PoolCounter configuration for Special:Contributions. $poolKey identifies the concurrency "pool" to draw from for this request; in this case it would contain the user ID or IP address, plus whatever constants are needed to not mix this with any other per-user pools. The doWork callback is called if the concurrency limit hasn't been reached, while error is called if it has.

You might also look at rMWf3819b6e2ecb: SpecialContributions: Use PoolCounter to limit concurrency as an example, as that added a per user/IP concurrency limit to a different code path.

Change 586322 had a related patch set uploaded (by Peter.ovchyn; owner: Peter.ovchyn):
[mediawiki/core@master] WIP: Add PoolCounterWork at the top of api entry point

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

@Anomie,
I've pushed temporary solutions. I don't think this is 100% good solution though.

There's a hook called before pooling starts working:
As long as there's pool exceeds the limit no need to call it.

Hooks::run( 'ApiBeforeMain', [ &$processor ] );

Placing all that logic inside the PoolCounter makes code quite complicated and untestable. I'm not sure it's a good option. In this case, it would be good to create another class and write tests on it, but ApiMain and ApiBase exist.

There's another option to put pooling under ApiMain::execute function. But this makes possible to override it in the hook.
So I doubt about best/simplest/reliable options here.

Anomie added a comment.Apr 6 2020, 2:10 PM

Neither api.php nor ApiMain are the locations being asked for. This task is asking for a narrower scope, concurrency for the parse in ApiParse.

Change 586322 merged by jenkins-bot:
[mediawiki/core@master] api: Wrap getParserOutput by PoolCounterWork in ApiParse module

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

Next step is to determine the limits and implement them for Wikimedia sites in a patch similar to rOMWC8274bc4f3b29: Add PoolCounter configuration for Special:Contributions.

'ApiParser' => [
    'class' => 'PoolCounter_Client',
    'timeout' => 15,
    'workers' => 3,
    'maxqueue' => ???,
],

A timeout of 15 seems sensible to match the timeout on ArticleView. 3 workers is (I think) what @Joe suggested in the task description. I don't know what to choose for maxqueue. It probably doesn't need to be too large because this is per-user.

Pchelolo closed this task as Resolved.Apr 15 2020, 4:13 PM