WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 6 weeks ago

#20537 reviewing defect (bug)

Don't spawn cron requests for suspended blogs

Reported by: ryan Owned by: jeremyfelt
Milestone: Future Release Priority: normal
Severity: normal Version: 3.3.1
Component: Cron API Keywords: has-patch needs-testing
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 5 years ago.
20537.2.patch (838 bytes) - added by kurtpayne 5 years ago.
Alternate patch
20537-with-filter.patch (2.8 KB) - added by jrf 21 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 18 months ago.
Updated docblocks
20537-with-filter-refreshed-complete.patch (2.7 KB) - added by jrf 18 months ago.
20537-6-with-filter.patch (3.2 KB) - added by jrf 16 months ago.
20537.2.diff (2.0 KB) - added by jeremyfelt 16 months ago.

Download all attachments as: .zip

Change History (33)

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

@ryan
5 years ago

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

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

@kurtpayne
5 years ago

Alternate patch

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

  • Focuses multisite added

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

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

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


19 months ago

#12 @jrf
18 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
18 months ago

Updated docblocks

#13 @jeremyfelt
17 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
16 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
16 months ago

#15 follow-up: @jeremyfelt
16 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
16 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
14 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
14 months 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.


12 months ago

#20 @jeremyfelt
12 months 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
11 months 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.


11 months ago

#23 @flixos90
11 months 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.


6 weeks ago

#25 @stevenkword
6 weeks 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 6 weeks ago by stevenkword (previous) (diff)

#26 @stevenkword
6 weeks ago

  • Keywords good-first-bug removed
Note: See TracTickets for help on using tickets.