Page MenuHomePhabricator

Confirm that post-send functions work fine with mediawiki under PHP 7
Closed, ResolvedPublic


Confirm that post-send functions work fine with mediawiki under PHP 7, per T176370

Event Timeline

BPirkle created this task.Nov 20 2018, 5:16 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 20 2018, 5:16 PM

Things I've done so far:

  1. Confimed DeferredUpdates::addCallableUpdate works with in both browser and command line modes on my local (vagrant) under PHP 7.0, 7.1, and 7.2. Not really much of a test, as neither register_postsend_function nor fastcgi_finish_request are available in my local, but at least it shows that things work happily in all those versions without any actual post send behaviors available.
  2. Confirmed DeferredUpdates::addCallableUpdate works in browser mode against under both hhvm and php7 (fpm-fcgi). php7 (fpm-fcgi) was triggered using the WMF browser extension.
  3. Confirmed DeferredUpdates::addCallableUpdate works against enwiki and testwiki under hhvm in command line mode via eval.php executed from mwmaint1002. Again, not much of a test - we already know it does - this was just to confirm my test code works as intended.

I confirmed browser mode by editing a page and checking/unchecking the "Watch this page" box. This triggers EditPage::updateWatchList(). The actual watchlist update is performed via a post-send function. So confirming whether the page appears/disappears from my watchlist tells me if the post-send function executed or not.

I confirmed command line mode by executing the following in eval.php:

$rps = function_exists( 'register_postsend_function' ) ? 'yes' : 'no'; $ffr = function_exists( 'fastcgi_finish_request' ) ? 'yes' : 'no'; DeferredUpdates::addCallableUpdate( function () use ( &$rps, &$ffr ) { echo "$rps|$ffr"; } );

Here is the same code, more readably formatted:

$rps = function_exists( 'register_postsend_function' ) ? 'yes' : 'no'; 
$ffr = function_exists( 'fastcgi_finish_request' ) ? 'yes' : 'no'; 
DeferredUpdates::addCallableUpdate( function () use ( &$rps, &$ffr ) { 
    echo "$rps|$ffr"; } 

The idea is to confirm the relevant environment, and that post-send behavior works in that environment, all in one command.

None of this is nearly as thorough as the tests performed in DeferredUpdateTest.php. Those tests, however, are limited in that they necessarily can't wait until the actual post-send, as they need to perform their assertions within the context of the phpunit test.

How useful is what I've done so far, does it actually do what I think it did, and what next steps should I take for additional confirmation? Is it possible in our current configuration to test command line mode under php7 on a production-ish environment?

Just noting one thing that was not obvious to me: in 7.1 php-fpm (the one I have, no idea about others) anything executed in post-send will still count towards the max_execution_time, so any post-send processing should be fast too.

Thank you @Nikerabbit . As part of testing T210567 locally on 7.2 php-fpm, I confirmed that behavior - my post-send processing was terminated by max_execution_time.

Krinkle added a subscriber: Krinkle.EditedDec 9 2018, 6:53 PM

@BPirkle That might be a problem. It is already a source of uncertainty to me whether our execution timeout honours things like finally, __destruct, and deferred updates. I've not yet found evidence that it causes corrupt state due to unexpected exists, though, in part because we use a transaction for all writes which means an early exist just effectively rolls it back. In theory this can still cause problems with secondary writes outside that transaction to other DBs, file backends, and caches; but our convention is to perform those writes after the transaction commits via db-idle callbacks.

However, if we also start aborting post-send callbacks, I believe it would likely cause corruption that does not self-correct because this is not a behaviour our architecture expects.

@Krinkle, by corruption, do you just mean things like aborted LinksUpdate operations as in T201482? PHP timeouts are expected to break things, the main solution should be to make things be fast enough or to increase the timeout. It seems to me that a missed LinksUpdate would not be a failure mode so severe as to be a blocker for PHP 7 deployment.

Further testing on production via the fatal-errors.php script from T210567 shows that timeout errors in a post-send function (which that script generates via infinite loop) occur on at 200 seconds under both HHVM and php7.

@tstarling Yeah, the main thing I was thinking about with corruption is things we put outside the main transaction that we still all expect to happen but don't have a retry or other eventual consistency mechanism for. In other words, deferred updates. They each get their own transaction.

If this was already getting aborted under HHVM then I suppose this isn't a blocker.

Having said that, the vast majority of adoption of post-send updates was introduced after we switched to HHVM and before we enabled execution timeouts (earlier this year). And at least for me it's a major surprise that these are aborted the same way. I do think it should be a priority to rethink this soon. E.g. disable the timeout in post-send (as a first measure), and for mid-term maybe remove it in favour of job queue entirely, or restructure it in a way that all deferred updates must be enqueable and maybe have some way for it to be enqueued but also ack'ed at the same time so that it tries to run it in post-send, but will also run from the regular job if it times out or otherwise not reach completion.

BPirkle closed this task as Resolved.Dec 13 2018, 3:11 PM

It seems that post-send functions work equivalently between php-fpm and hhvm. So I'm closing this task and marking it as complete on the php7 checklist. If we want to investigate/adjust timeouts and look at related concerns (queuing, etc.), let's do that under a separate task.