Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 23 months ago

#54668 closed enhancement (fixed)

Non blocking wp-cron.php with LSAPI

Reported by: maximej's profile maximej Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.1 Priority: normal
Severity: normal Version:
Component: Cron API Keywords: has-patch add-to-field-guide
Focuses: performance Cc:

Description

As a follow-up to that change [44488] (Make wp-cron.php non-blocking where possible)

Currently, it's only done with FastCGI, but it's also possible with LiteSpeed LSAPI using litespeed_finish_request().

fastcgi_finish_request() was an alias for a short time in LSAPI but it's not anymore, so wp-cron.php is blocking with LSAPI.

Could we consider implementing it in wp-cron.php?

Others already done that: https://github.com/symfony/symfony/pull/42332
and I don't see any issues using it in wp-cron.php.

Change History (11)

#1 @johnbillion
3 years ago

  • Keywords needs-patch added
  • Version trunk deleted

Welcome @maximej and thanks for the ticket.

This sounds like it makes sense. PRs are welcome against https://github.com/WordPress/wordpress-develop.

Could you provide the testing steps necessary please?

#2 @maximej
3 years ago

Thanks @johnbillion, will do a PR soon (not familiar with the testing steps however).

But before, it leads me to another point :
With #47396 and the following changesets [45560] and [46458]
no cache headers are sent for these "early finished requests"

if ( ! headers_sent() ) {
  header( 'Expires: Wed, 11 Jan 1984 05:00:00 GMT' );
  header( 'Cache-Control: no-cache, must-revalidate, max-age=0' );
}

But it was stated that calls to wp-cron.php are not intended to be cached, so why sending these headers only on that non blocking case ?
wp-cron.php can be cached if hosted on incompatible servers.

Do I miss something @peterwilsoncc?

I get that

the fastcgi_finish_request() call prevents the setting of further HTTP headers by caching plugins subsequently loaded.

but no non caching headers are sent afterwards by default.

This ticket was mentioned in PR #2155 on WordPress/wordpress-develop by Maxime-J.


3 years ago
#3

  • Keywords has-patch added; needs-patch removed

LSAPI support added for a non-blocking wp-cron.php
Headers intended to prevent caching moved outside of the non-blocking scenario, so that they are also sent if non-blocking isn't supported

Trac ticket: https://core.trac.wordpress.org/ticket/54668

#4 @maximej
2 years ago

Hi @johnbillion @peterwilsoncc
Sorry if I am bothering you, but any news on that being considered?

Support for non blocking in LSAPI set appart, the fact that headers to prevent caching are only sent with FastCGI looks like a problem to me. Do I miss something?

#5 @peterwilsoncc
2 years ago

  • Milestone changed from Awaiting Review to 6.1

@maximej I've just updated your pull request to resolve some merge conflicts.

I've put this on the milestone for the next major version of WordPress.

Do you know which version of the LSAPI removed the fastcgi_finish_request() alias? It's not in the release log and I am wondering if there was a bug with ending requests early that the developers were avoiding.

#6 @peterwilsoncc
2 years ago

I get that (fastcgi_finish_request prevents setting further headers but) no non caching headers are sent afterwards by default.

Sorry, I missed this question earlier today.

The idea behind placing the headers inside the fastcgi_finish_request was to ensure that any full page caching plugins retained control of how to manage the wp-cron cache. Most of them do a check for DOING_CRON === true and send out their own headers.

To avoid repeating code multiple times, I think the change you have in your pull request is for the best. Caching plugins calling header() with expiry headers later in the process will replace the existing headers.

#7 @peterwilsoncc
2 years ago

  • Owner set to peterwilsoncc
  • Resolution set to fixed
  • Status changed from new to closed

In 53653:

Cron API: Make wp-cron non-blocking for LiteSpeed LSAPI.

This should make cron spawning faster on LSAPI by ensuring that cron requests return immediately.

To avoid code repetition, the no caching headers are relocated and always sent. Caching plugins attempting to set these
headers later will replace those set by WordPress Core.

Follow up to [44488].

Props maximej, johnbillion.
Fixes #54668.

peterwilsoncc commented on PR #2155:


2 years ago
#8

Merged in 1ff92618f22b92683ca85bff30f368f76d86d9c5 -- canonically https://core.trac.wordpress.org/changeset/53653

#9 @maximej
2 years ago

Thanks for the change and the explanation @peterwilsoncc !

Concerning the alias removal, here is the according commit
https://github.com/php/php-src/commit/ccf051c317e606c2a4d9099c6e79a5c42bfdb298
just a few days after they released litespeed_finish_request()

Apparently, there was indeed a bug, fixed in that commit. That may be why they removed the alias, as a precaution.
It was stated "for now" in the commit, but the alias actually never went back in.

This ticket was mentioned in Slack in #hosting-community by javier. View the logs.


2 years ago

#11 @milana_cap
23 months ago

  • Keywords add-to-field-guide added
Note: See TracTickets for help on using tickets.