WordPress.org

Make WordPress Core

Opened 11 months ago

Last modified 10 months ago

#41358 new defect (bug)

Shutdown hooks can significantly slow down REST API responses

Reported by: mikejolley Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: REST API Keywords: has-patch dev-feedback needs-testing
Focuses: rest-api, performance Cc:

Description

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.

Attachments (1)

end-rest-api-connection-41358.diff (1.3 KB) - added by mikejolley 11 months ago.
Patch for issue 41358

Download all attachments as: .zip

Change History (8)

@mikejolley
11 months ago

Patch for issue 41358

#1 @mikejolley
11 months ago

  • Keywords has-patch dev-feedback needs-testing added

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.

#2 @johnbillion
11 months ago

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?

#4 @jnylen0
11 months ago

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.

#5 @tellyworth
11 months ago

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.

#6 @rmccue
10 months ago

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.

#7 @mikejolley
10 months ago

@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...

Note: See TracTickets for help on using tickets.