Make WordPress Core

Opened 23 months ago

Last modified 3 weeks ago

#55999 new defect (bug)

wp_suspend_cache_addition should also disable cache setting?

Reported by: malthert's profile malthert Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Cache API Keywords: has-patch 2nd-opinion has-unit-tests early
Focuses: performance Cc:

Description

Right now wp_suspend_cache_addition is only checked when "add" to cache.

However most plugins/developers only use wp_cache_set for updating/adding to the cache.
This means in most plugins/cases, wp_suspend_cache_addition has no use, in fact it's rather unexpected behavior that only some functions can add to cache (namely: set, incr, decr)

I propose we also check wp_suspend_cache_addition before setting the cache.
This would also be in line with its function description:

Stops more data being added to the cache, but still allows cache retrieval.

As "set" also "adds" (or at least modifies if already exists therefore adds/subtracts) data in cache.

Change History (6)

This ticket was mentioned in PR #2824 on WordPress/wordpress-develop by kkmuffme.


23 months ago
#1

  • Keywords has-patch added

#2 @johnbillion
23 months ago

  • Focuses performance added
  • Keywords needs-unit-tests 2nd-opinion added
  • Version trunk deleted

This is a good idea but it is a breaking change. During a large import for example, suspending cache addition makes sense to avoid consuming a large amount of memory (particularly when inserting terms and post meta), but updating existing cache entries may well be desirable and expected.

I'll see if I can find some concrete examples.

I think to avoid backwards compatibility issues this needs to be implemented via a new function such as wp_suspend_cache_changes().

Either way, it would be good to get test coverage on this too.

#3 @kkmuffme
3 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

During a large import for example, suspending cache addition makes sense to avoid consuming a large amount of memory (particularly when inserting terms and post meta), but updating existing cache entries may well be desirable and expected.

  • for incr/decr it will end up with wrong values if only "add" is suspended but you then call incr/decr on it. For these 2 it's breaking but the original behavior was unintended and there isn't a use case I can think of where this is expected.
  • for set: it's breaking what is currently broken = making it work as expected :-) when using a persistent cache backend. The current behavior is a massive source of race conditions causing invalid cache states.

Additionally, from the function description of the suspend function it reads as it will suspend all "adding" to cache - that it previously only stopped "add" is mostly unexpected I'd say.

I'm also not really able to come up with any example where you want to disable "add" but still be able to "set"?

---

Added unit tests now

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


3 months ago

#5 @swissspidy
3 months ago

  • Keywords early added
  • Milestone changed from Awaiting Review to Future Release

Adding the early keyword as this kind of change should be potentially done earlier in a cycle.

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


3 weeks ago

Note: See TracTickets for help on using tickets.