WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#23448 closed defect (bug) (fixed)

Passive invalidation via last_changed should use timestamps

Reported by: westi Owned by: ryan
Milestone: 3.6 Priority: high
Severity: major Version: 3.6
Component: Cache API Keywords: has-patch
Focuses: Cc:

Description

For #23173 we are switching lots of things over to passive invalidation via cache incrementors and last_changed.

This includes switching the oldest thing we had using this method of invalidation over to using wp_cache_incr.

This ticket is a petition to use time() still as the initial value so as to avoid race conditions with cache evictions and cache incrementors.

Currently with a 1 based last_changed key the following is possible with a persistent object cache backend like memcache:

  • last_changed is set to 1
  • list of pages is cached with 2 pages being the result of a get_pages call
  • last_changed is evicted from the memcache server
  • Another page is added
  • A call to get_pages happens, sets last_changed to 1 and returns the old data

Confusion the reigns for a bit until other things are evicted or another change is made.

If we start with last_changed = time() then we have no danger of using stale cache entries.

Attachments (1)

23448.diff (3.5 KB) - added by westi 4 years ago.
Switch to time() based last_changed

Download all attachments as: .zip

Change History (13)

@westi
4 years ago

Switch to time() based last_changed

#1 @westi
4 years ago

  • Keywords has-patch added; needs-patch removed

#2 @westi
4 years ago

This patch should fix all these issue, it breaks all the tests that assume we start from 1 and increment up though ;)

#3 follow-up: @ryan
4 years ago

How about microtime()?

#4 @ryan
4 years ago

A reminder that a bunch of unit tests will need to update. I can get those.

#5 @mark-k
4 years ago

+1000, but anything random enough should be good.

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

Replying to ryan:

How about microtime()?

That would work too, anything that doesn't always start somewhere deterministic is what matters :)

#7 @ryan
4 years ago

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

In 23401:

Use microtime() instead of incrementors for last_changed to to avoid race conditions with cache evictions.

Props westi
fixes #23448

#11 @nacin
3 years ago

In 27115:

Use a float for last_changed microtime cache values.

microtime() by default returns a string with a space, which isn't allowed for keys in some cache backends.

props _jameslee, drozdz.
fixes #27000. see #23448.

#12 @nacin
3 years ago

In 27300:

Revert [27115] and let cache backends handle the stripping of spaces in cache keys as necessary.

microtime() returns greater precision than microtime(true).

see #27000, #23448, #26903, #14485.

Note: See TracTickets for help on using tickets.