Make WordPress Core

Opened 12 years ago

Closed 3 years ago

#20537 closed defect (bug) (fixed)

Don't spawn cron requests for suspended blogs

Reported by: ryan's profile ryan Owned by: jeremyfelt's profile jeremyfelt
Milestone: 5.7 Priority: normal
Severity: normal Version: 3.3.1
Component: Cron API Keywords:
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 12 years ago.
20537.2.patch (838 bytes) - added by kurtpayne 12 years ago.
Alternate patch
20537-with-filter.patch (2.8 KB) - added by jrf 8 years 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 8 years ago.
Updated docblocks
20537-with-filter-refreshed-complete.patch (2.7 KB) - added by jrf 8 years ago.
20537-6-with-filter.patch (3.2 KB) - added by jrf 8 years ago.
20537.2.diff (2.0 KB) - added by jeremyfelt 8 years ago.

Download all attachments as: .zip

Change History (37)

#1 @ryan
12 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 12 years ago by ryan (previous) (diff)

@ryan
12 years ago

#2 @nacin
12 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
12 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
12 years ago

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

@kurtpayne
12 years ago

Alternate patch

#5 @kurtpayne
12 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
12 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
9 years ago

  • Focuses multisite added

#8 @swissspidy
8 years 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
8 years ago

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

#9 @jrf
8 years 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
8 years 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.


8 years ago

#12 @jrf
8 years 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
8 years ago

Updated docblocks

#13 @jeremyfelt
8 years 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
8 years 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
8 years ago

#15 follow-up: @jeremyfelt
8 years 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
8 years 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
8 years 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 years 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.

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


8 years ago

#20 @jeremyfelt
8 years ago

  • Keywords 4.7-early removed
  • Milestone changed from Future Release to 4.7
  • Owner changed from jrf to jeremyfelt
  • Status changed from assigned to reviewing

#21 @jorbin
7 years ago

@jeremyfelt Still targetting this for 4.7? If so, what do you need?

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


7 years ago

#23 @flixos90
7 years ago

  • Milestone changed from 4.7 to Future Release

This might be close, but there hasn't been any activity lately, and it still needs some testing. Let's get back to it for the next release.

This ticket was mentioned in Slack in #core-multisite by stevenkword. View the logs.


7 years ago

#25 @stevenkword
7 years ago

20537.2.diff still applies cleanly against trunk as of Jul 10, 2017. However, further user testing is still required. New contributors looking to get involved could provide a much-needed benefit through some more user testing.

@jeremyfelt -- Do you still feel further discussion around this new filter is required?

Last edited 7 years ago by stevenkword (previous) (diff)

#26 @stevenkword
7 years ago

  • Keywords good-first-bug removed

#27 @peterwilsoncc
6 years ago

  • Keywords needs-refresh added

Needs refresh:

  • no longer applies cleanly following coding standards fix
  • reversed logic noted in comment 16
  • to use get_site() or get_blog_details()

@flixos90 and @jeremyfelt is this something you'd still like to get in?

#28 @peterwilsoncc
3 years ago

  • Milestone changed from Future Release to 5.7

Moving this to the 5.7 milestone as it will be fixed as a side-effect of #24160 if that goes in.

#29 @peterwilsoncc
3 years ago

  • Keywords has-patch needs-testing needs-refresh removed

For #24160, I'm moving alternative wp-cron to run on the wp_loaded hook.

Having reviewed what happens on multisite for regular cron, hitting /site-name/wp-cron.php already shuts down prior to jobs running unless the user is logged in. For loopback requests this is not the case.

Plugins can use the ms_site_check filter to prevent suspended blogs from shutting down, in which case both standard and alternative cron will continue to fire.

Given the option exists (and I am unsure if it did when this ticket was last discussed) I'm moving the hook without considering the edge case above.

#30 @peterwilsoncc
3 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 50135:

Cron API: Run alternative wp-cron later, do not run on archived blogs.

Runs cron jobs later on sites using alternative cron, ie the ALTERNATE_WP_CRON constant is true, to more closely match when standard cron jobs are run. Jobs now run on the wp_loaded hook at priority 20. Prior to this change they would run on the init hook. This ensures custom post types and taxonomies are registered prior to the jobs running.

This change also prevents alternative wp-cron from running on archived or suspended multisite blogs as these are shut down prior to the wp_loaded hook from running.

Moves the existing functionality of wp_cron() in to a new private function _wp_cron().

Props flixos90, jeremyfelt, johnbillion, jrf, kurtpayne, nacin, peterwilsoncc, prettyboymp, r-a-y, ryan, stevenkword, swissspidy.
Fixes #20537, #24160.

Note: See TracTickets for help on using tickets.