WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 8 weeks ago

#20537 assigned defect (bug)

Don't spawn cron requests for suspended blogs

Reported by: ryan Owned by: jrf
Milestone: Future Release Priority: normal
Severity: normal Version: 3.3.1
Component: Cron API Keywords: has-patch good-first-bug needs-testing 4.7-early
Focuses: multisite Cc:

Description (last modified by swissspidy)

For multisite, spawning cron requests is wasteful and unnecessary. wp_cron()/spawn_cron()/wp-cron.php should check the blog's status and bail if the blog is suspended.

Attachments (7)

20537.diff (481 bytes) - added by ryan 4 years ago.
20537.2.patch (838 bytes) - added by kurtpayne 4 years ago.
Alternate patch
20537-with-filter.patch (2.8 KB) - added by jrf 9 months ago.
Refreshed patch, including filter which allows to force skip/force run cron anyhow.
20537-with-filter-refreshed.patch (1.1 KB) - added by jrf 6 months ago.
Updated docblocks
20537-with-filter-refreshed-complete.patch (2.7 KB) - added by jrf 6 months ago.
20537-6-with-filter.patch (3.2 KB) - added by jrf 5 months ago.
20537.2.diff (2.0 KB) - added by jeremyfelt 4 months ago.

Download all attachments as: .zip

Change History (25)

#1 @ryan
4 years ago

Cribbing from ms_site_check(), the conditions to check:

if ( '1' == $current_blog->deleted || '2' == $current_blog->deleted || '1' == $current_blog->archived || '1' == $current_blog->spam )
 return;

Or from get_blogs_of_user():

if ( $blog->archived || $blog->spam || $blog->deleted )
     return;
Last edited 4 years ago by ryan (previous) (diff)

@ryan
4 years ago

#2 @nacin
4 years ago

I think spawning is unnecessary. wp-cron.php should still be able to be triggered, though. Someone somewhere is surely doing something weird with regards to this.

I think it would be best if bailed early in spawn_cron() and didn't hook wp_cron() directly into sanitize_comment_cookies. That would allow for wp_cron() and wp-cron.php to still execute.

I could also just be overly careful.

#3 follow-up: @ryan
4 years ago

  • Description modified (diff)

I can see it being good and bad, at least when considering core events. A hard freeze prevents future posts from accidentally getting published and pingbacks from accidentally going out from a blog that should be dead. The blog and the cron queue are frozen. Then again, there could be some events that should still be processed. This was requested by some wp.com folks to shut up zombie blogs. How dead is dead enough?

#4 @ryan
4 years ago

And, yes, spawning should be shut down and the hook avoided. Finishing the patch tomorrow.

@kurtpayne
4 years ago

Alternate patch

#5 @kurtpayne
4 years ago

  • Cc kpayne@… added
  • Keywords has-patch 2nd-opinion added

20537.2.patch is the same as 20537.diff, but in a different place. It prevents the cron request from spawning for zombie blogs, but still allows wp-cron.php to be called directly.

Still a bit new to the cron code. Is this right?

#6 in reply to: ↑ 3 @nacin
4 years ago

Replying to ryan:

How dead is dead enough?

I'm convinced. Both spawn_cron() and wp-cron should bail. Maybe a filter in wp-cron for the creative.

#7 @jeremyfelt
18 months ago

  • Focuses multisite added

#8 @swissspidy
9 months ago

  • Keywords needs-refresh good-first-bug added; 2nd-opinion removed

20537.2.patch is a good start. Needs a refresh using a new filter, so people could still spawn cron requests for these blogs when needed.

@jrf
9 months ago

Refreshed patch, including filter which allows to force skip/force run cron anyhow.

#9 @jrf
9 months ago

  • Keywords needs-refresh removed

New patch uploaded which combines the previous two patches and adds a filter as suggested.

Both previous patches are needed if we want to bail early:

  • wp_cron() would be the earliest point of entry for the poor mans cron run
  • wp-cron.php for those of us who run real cron jobs.

#10 @swissspidy
9 months ago

  • Description modified (diff)
  • Keywords needs-testing added
  • Owner set to jrf
  • Status changed from new to assigned

Marking this "good-first-bug" as claimed.

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


7 months ago

#12 @jrf
6 months ago

The patch is a good start. The docbloc needs to be adjusted to show the global $current_blog added into wp_cron and the new filter is missing a version number in the docbloc

Updated patch for feedback from Slack chat.

@jrf
6 months ago

Updated docblocks

#13 @jeremyfelt
5 months ago

  • Milestone changed from Future Release to 4.6

Thanks for the patches, @jrf, this looks close.

  • Let's use get_blog_details() instead of accessing the global $current_blog. See #22090.
  • How about naming the filter skip_cron_for_site and not using the "blog" terminology here?
  • Now that WP_Site is available, the filter docs should be adjusted to show that a WP_Site object is provided.

#14 @jrf
5 months ago

Hi @jeremyfelt

Thanks for the patches, @jrf, this looks close.

  • Let's use get_blog_details() instead of accessing the global $current_blog. See #22090.
  • How about naming the filter skip_cron_for_site and not using the "blog" terminology here?
  • Now that WP_Site is available, the filter docs should be adjusted to show that a WP_Site object is provided.

Thanks for the feedback.

I've adjusted the patch. Changes include:

  • Switched over to using get_blog_details() as per your suggestion. This does mean an extra level of nesting as the function is only available when on multi-site.
  • Renamed the filter. Some consistence in the naming here would be awesome, get_blog_details() versus the use of site in the filter can lead to confusion.
  • Adjusted the documentation to mention WP_Site.
  • Adjusted the documentation to make it extra clear when a site is regarded as suspended.
  • Updated the @since tag for the filter to 4.6.0.

Little side-note: I would expect WP_Site->public to be false (0) for suspended blogs, but it still shows as 1 (true).

@jeremyfelt
4 months ago

#15 follow-up: @jeremyfelt
4 months ago

Thanks, @jrf, I think we're getting close.

20537.2.diff makes a couple changes:

  • Use $spawn_cron as the variable tracking whether or not to spawn. Default to true rather than false.
  • Name the filter spawn_cron_for_site and adjust filter docs accordingly.

Pinging @DrewAPicture to have a look at the filter too. It feels a little weird to either pass null or a WP_Site instance, but that might be all we have.

#16 @jrf
4 months ago

@jeremyfelt

If you reverse the variable logic, please do so consistently:
In src/wp-cron.php you need to change true to false in the following line:

if ( true === apply_filters( 'spawn_cron_for_site', $spawn_cron, $current_site ) ) {
        die();
}

I also believe that the additional documentation in the filter about when a site is regarded as suspended should be put back in as this may be clear to us, but not automatically so for other people without looking at the actual code.

#17 in reply to: ↑ 15 @flixos90
2 months ago

Replying to jeremyfelt:

It feels a little weird to either pass null or a WP_Site instance, but that might be all we have.

Looking at this, I think it would probably make sense to only run this filter on a Multisite setup and otherwise always have $spawn_cron set to true. This way we can always pass a WP_Site instance, and on a single site, having a filter like this doesn't sound useful to me (as you could just DISABLE_WP_CRON there).

Replying to jrf:

In src/wp-cron.php you need to change true to false in the following line:

Yep - we all make mistakes. :)

Another minor update needed, let's use get_site() instead of get_blog_details(), now that that function is available.

#18 @jeremyfelt
8 weeks ago

  • Keywords 4.7-early added
  • Milestone changed from 4.6 to Future Release

I'm going to move this to 4.7-early. This is a long standing bug and we shouldn't be introducing a new filter this late without some more discussion.

Note: See TracTickets for help on using tickets.