Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send headers when shutting down. #16158

Closed
wants to merge 1 commit into from
Closed

Conversation

mikejolley
Copy link
Member

Sends headers on the shutdown hook so expensive shutdown actions do not slow things down with the REST API.

e.g. adding a product, on shutdown there may be sync events. This ensures the response was sent before that happens.

session_write_close();
}

// This is the cleanest approuch when available and is used on nginx.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo approuch. Also, it's not exclusive to nginx, it's part of the php-fpm package which can be installed as an Apache module too (it just happens that my installation doesn't include it by default). I think it's enough to say that "it isn't always present".

@@ -23,6 +23,9 @@ class WC_API extends WC_Legacy_API {
public function __construct() {
parent::__construct();

// Wrangle long requests.
add_action( 'shutdown', array( $this, 'rest_send_reponse_before_shutdown' ), -1 );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it make more sense to add this to Jetpack instead, since the slow shutdown hooks are inside Jetpack? @beaulebens ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the slowdown is coming from Jetpack then yes, certainly we want to fix it there.


// For apache, send some headers.
} else {
header( 'Content-Length: ' . ob_get_length() );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably want to wrap these in a headers_sent() check, otherwise it'll throw errors if any output has already been sent.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@beaulebens line 52, along with checks to ensure this is a REST api request.

@mikejolley
Copy link
Member Author

@mikejolley
Copy link
Member Author

Submitted a patch to core https://core.trac.wordpress.org/ticket/41358#comment:1

@mikejolley mikejolley closed this Aug 9, 2017
@mikejolley mikejolley deleted the fix/slow-rest-connections branch August 9, 2017 09:35
@mikejolley mikejolley removed this from the 3.1.2 milestone Aug 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants