Shutdown hooks can significantly slow down REST API responses

[mikejolley]

If you have a site with some slow, maybe deferred actions hooked into shutdown, these actions will slow the response from the REST API unnecessarily.

To test, simply add the following then do a request to the WP Rest API:

<?php
add_action( 'shutdown', 'delay_shutdown' );

function delay_shutdown() { sleep( 3); }

The response will not be sent out until all of these shutdown hooks are completed. In our case, we found that some heavy sync functions ran on shutdown would cause our API calls to WooCommerce API timed out, when really the response could have been sent much earlier so these calls could run afterwards.

This is semi-related to https://core.trac.wordpress.org/ticket/27122 in that the fix could be along those lines.

This isn't a solution we feel should be merged into WooCommerce itself because it would be more consistent to have it in WP itself, but see ​https://github.com/woocommerce/woocommerce/pull/16158/files

So on shutdown, it ends any sessions, uses fastcgi_finish_request if available, and falls back to flushing. The response is sent and received, and shutdown continues on in the background. The response is sent before the sleep(3) in this test case.

This could be applied to just REST API requests, or everywhere, depending on everyones feedback. If REST API only, it may make sense to fire an action after the response is sent to avoid shutdown hook altogether.

[mikejolley]

Patch for issue 41358

[mikejolley]

I've added a REST API specific patch since this has the lowest barrier to entry, resolves the problem where it's most noticeable, and should be easier to test.

https://core.trac.wordpress.org/attachment/ticket/41358/end-rest-api-connection-41358.diff

New method uses fastcgi_finish_request if possible otherwise defaults to sending headers.

To see how much of an improvement it makes, use the code in the original post. If you make a request to the API, before patch you'll wait at least 3 seconds to see the response. After patch, the request is sent instantly.

[johnbillion]

I'm going to ping @tellyworth here because he has experience with fastcgi_finish_request() on dot com. Alex, have you seen any problems relating to this?

[johnbillion]

See also: https://core.trac.wordpress.org/ticket/18738#comment:30

[jnylen0]

I tested a few things locally:

• On nginx, fastcgi_finish_request is called; error logging during a long shutdown hook seems to work fine, contrary to some of the above reports (may depend on PHP version?)
• On apache, the logic to send Connection: close and other headers is called, and works fine.

As noted at ​https://stackoverflow.com/a/27847878/106302, on both nginx and apache, outputting after finish_request does nothing, unless output is flushed (using flush() for example), in which case the script will abort. This would be pretty terrible to debug, because the abort doesn't appear to happen immediately.

Fortunately, as noted at ​https://bugs.php.net/bug.php?id=68772, this is fixed for both webservers by calling ignore_user_abort( true ); during finish_request.

I also think we should change the Apache logic a bit: how do we know that ob_get_length() is the correct content length? Maybe we can have serve_request always send a Content-Length header based on the data it echoes, and finish_request do the rest.

[tellyworth]

There's a PHP bug that looks relevant: ​https://bugs.php.net/bug.php?id=67583

It's since been fixed, but could be a problem with certain PHP versions.

[rmccue]

I'm not sure that this is really REST API-specific. Generally speaking, shutdown hooks will delay any response (and that might be desired behaviour too).

I'd rather have the REST API match regular WordPress behaviour, so I think any solution should be consistent across all of core.

[mikejolley]

@rmccue The difference in my eyes is that when the REST api sends the responses, thats it. Nothing else should be sent. Doing so would be invalid.

So this improves performance without affecting other parts of core. I imagine it will take much longer to get approval for a WP-wide change...

[tellyworth]

In 44488:

Cron API: Make wp-cron.php non-blocking where possible.

This should make cron spawning faster by ensuring requests to wp-cron.php return immediately regardless of transport method. It is enabled only on recent PHP versions with fastcgi, due to historical bugs and availability of fastcgi_finish_request(). This needs testing on a range of platforms, to help determine if it's safe to use in other contexts also.

Props vnsavage, johnbillion, jnylen0.
See #18738, #41358

[johnbillion]

@tellyworth One side effect of calling fastcgi_finish_request() is that any output produced by a shutdown function (or a function hooked to the shutdown action) will not be visible, as the request has already been finished.

I noticed this with another application that was calling fastcgi_finish_request() and caused Query Monitor to break, as its output is produced on the shutdown action and therefore wasn't returned in the response.

This is not a concern for wp-cron.php but it's definitely worth bearing in mind for other contexts.

[pbearne]

Is there any work needed on this ticket?