Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 6 years ago

#41699 closed defect (bug) (fixed)

Expired transients are not flushed by cron

Reported by: bor0's profile bor0 Owned by: dd32's profile dd32
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.8.1
Component: Options, Meta APIs Keywords: has-patch
Focuses: Cc:

Description

Hi there.

Is there a special reason why we delete expired transients only on get_transient attempt (https://github.com/WordPress/WordPress/blob/master/wp-includes/option.php#L662), but not in cron.php?

I believe https://github.com/WordPress/WordPress/blob/master/wp-admin/includes/upgrade.php#L1748-L1759 belongs in cron.php. Otherwise we're having leftover transients until it's attempted to retrieve them.

I will assume two cases from here:

  • It was done due to performance reasons. In which case there could've been a button in the wp-admin somewhere to flush the expired transients, or allow the users to enable/disable cron deletion through a config param in wp-config.php.
  • It was a bad design decision. In which case we can copy the excerpt from upgrade.php to cron.php somewhere.

In any case, I believe the fix is easy and doesn't have a big impact, while at the same time it allows us to keep the DB from being polluted.

Attachments (4)

41699.diff (725 bytes) - added by Mte90 8 years ago.
41699.2.diff (9.6 KB) - added by dd32 8 years ago.
41699.3.diff (12.3 KB) - added by dd32 8 years ago.
41699.4.diff (1.6 KB) - added by dlh 8 years ago.

Download all attachments as: .zip

Change History (23)

#1 @bor0
8 years ago

  • Keywords dev-feedback added

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


8 years ago

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


8 years ago

#4 @SergeyBiryukov
8 years ago

  • Milestone changed from Awaiting Review to 4.9

@Mte90
8 years ago

#5 @Mte90
8 years ago

Looking at the code there is already a loop of all the hooks with unschedule (https://github.com/WordPress/WordPress/blob/master/wp-cron.php#L115).
But unschedule is used to remove in the system the scheduled hooks and not cleaning.

Looking at the code in upgrade.php the query is running for a multisite environment but if we are running wp-cron.php we have to act on the cron stuff of the site that is running.

So looking on the code of a famous wordpress plugin for clean the transient (https://plugins.trac.wordpress.org/browser/delete-expired-transients/trunk/includes/class.DelxtransCleaners.php) I made that patch that run the query before the die(); of wp-cron.

#6 @bor0
8 years ago

  • Keywords has-patch added; dev-feedback removed

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


8 years ago

@dd32
8 years ago

#8 @dd32
8 years ago

The correct code to copy for this is the section from schema.php: https://github.com/WordPress/WordPress/blob/master/wp-admin/includes/schema.php#L587-L607

For some background on when the cleanup code was added, check out #20316

At the time, it wasn't uncommon for people to be abusing transients, but it was rare for it to be causing issue.
Clearing on DB upgrade was acceptable as it didn't need to happen *that* often, once every few months was plenty.

Unfortunately since adding that code, more plugins have abused it in even more wonderful ways, passing even more unique keys through to transients.. causing this need for it to occur more often.

One of the suggestions from #20316 was for it to be part of wp_scheduled_delete() which cleans up the Trash'd posts/comments.
41699.2.diff is a mostly untested first scratch at such a patch putting it into it's own function. The patch queues the job up in admin.php which doesn't seem like the best location, but does mean that it'll only be added for existing sites that are used.

#9 @Mte90
8 years ago

Probably is better a cron that daily clean up the unflushed transients and avoid that the admin do this kind of operation.

#10 @westonruter
8 years ago

  • Owner set to dd32
  • Status changed from new to assigned

Who is owning this to fix for 4.9? I'll assign to @dd32 since he uploaded the last patch.

Closely related to #38899 (Deletion of auto-drafts and trashed posts never gets scheduled unless user accesses admin pages).

@dd32
8 years ago

#11 @dd32
8 years ago

Here's another partially tested patch. 41699.3.diff Not completely happy with it, and can't really test it, so I'm going to leave it here for a bit longer in the hope someone else weighs in.

#12 @westonruter
8 years ago

@dd32 what specifically aren't you happy with?

#13 @dd32
8 years ago

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

In 41963:

Transients: Clear expired transients from the database in a daily cron task.

Fixes #41699

@dlh
8 years ago

#14 @dlh
8 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I apologize if it was intentional, but [41963] left out the new wp_schedule_event() for 'delete_expired_transients'. 41699.4.diff would add it back and fixes a couple of formatting glitches with the delete_expired_transients() function.

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


8 years ago

#16 @johnbillion
8 years ago

  • Status changed from reopened to reviewing

#17 @SergeyBiryukov
8 years ago

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

In 42008:

Transients: After [41963], add missing cron task for delete_expired_transients().

Props dlh.
Fixes #41699.

#18 @dd32
7 years ago

Thanks @SergeyBiryukov! (Sorry folks, was offline & missed the file in the checkin)

#19 @dartiss
6 years ago

#45277 was marked as a duplicate.

Note: See TracTickets for help on using tickets.