WordPress.org

Make WordPress Core

Opened 6 months ago

Closed 3 weeks ago

#25773 closed defect (bug) (fixed)

can not update option in wp 3.7 and hanging in wp_clear_scheduled_hook()

Reported by: dimagsv Owned by: nacin
Milestone: 3.7.2 Priority: high
Severity: normal Version: 3.7.1
Component: Cache API Keywords: fixed-major
Focuses: Cc:

Description

update_option() must update option in memory cache and in database, but updates only in database. New option value not updated in memory cache when value in database is equal to new option value bacause zero (0) rows affected and update_option() return error (false).
wp_clear_scheduled_hook() hanging because cannot delete value from 'cron' option due this bug in update_option().
I think this code in update_option()

if ( ! $result )
 return false;

should be replaced with

if ( $result === false )
 return false;

Attachments (1)

25773.diff (634 bytes) - added by dd32 4 months ago.

Download all attachments as: .zip

Change History (14)

comment:1 SergeyBiryukov6 months ago

  • Milestone changed from Awaiting Review to 3.7.2

Introduced in [25664].

comment:2 nacin6 months ago

New option value not updated in memory cache when value in database is equal to new option value bacause zero (0) rows affected and update_option() return error (false).

I think we need a test case. If the database had zero rows affected, then nothing was updated, and there is nothing to update in memcached.

comment:3 dimagsv6 months ago

How to reproduce.

  1. Install Google XML Sitemaps plugin.
  2. Activate plugin.
  3. Enable option 'Rebuild sitemap if you change the content of your blog'.
  4. Open in admin area any published page or post, press 'Save'.
  5. Wait at least 15 seconds. Better 20 seconds. Do not open any other pages of this wordpress installation during this 15-20 seconds.
  6. Press 'Save' again. WP hangs at wp_clear_scheduled_hook().

What is happen.

  1. The event 'sm_build_cron' was scheduled on current time + 15 seconds after the first press 'Save'.
  2. Options was read from database with cron scheduled event 'sm_build_cron' after the second press 'Save'.
  3. Cron job started in the new thread.
  4. Cron job in the new thread clear event 'sm_build_cron' from database earlier than wp_clear_scheduled_hook() called in the main thread.
  5. Event 'sm_build_cron' exist in the main thread memory cache, but not in the database.

comment:4 follow-up: dd325 months ago

  1. Cron job in the new thread clear event 'sm_build_cron' from database earlier than wp_clear_scheduled_hook() called in the main thread.
  2. Event 'sm_build_cron' exist in the main thread memory cache, but not in the database.

So we've got 2 bugs here, based around the same thing,

  1. If wp_clear_scheduled_hook() has been run in another thread, wp_clear_scheduled_hook() will endlessly loop in the current thread, because:
  2. update_option() may now fail in "high-traffic" situations where an option is being updated my multiple threads to the same value.

There's multiple fixes:

  1. Revert [25664], but that has the disadvantage that a DB write failing will still end up in the cache
  2. make wp_clear_scheduled_hook() not rely upon wp_next_scheduled(), resulting it only trying to unset/save once-per-event rather than looping (because the cache says it's still there after "clearing" it)
  3. make wp_next_scheduled() bypass the cache when it calls _get_cron_array() so that it's always got the latest data from the DB, downside that it's not as performant
  4. make wp_clear_scheduled_hook aware that wp_unschedule_event() failed (but that also means making wp_unschedule_event() aware that _set_cron_array() can fail, and making _set_cron_array() aware that update_option can fail)
  5. In the event that update_option() fails, populate the local cache with fresh data from the DB - this is technically possible, but also potentially weird, in that update_option might fail due to a DB issue, and then read stale data from a slave DB, or another write between the attempted write and the read causes the value to change to another one.

It's a tricky situation, as WordPress generally relies upon the fact that data it's using won't be altered outside of the current thread while it's using it.. which is a really bad operating assumption to make.

A unit test might be hard for this scenario, but should be able to replicate it by modifying the DB directly..

comment:5 samuelsidler5 months ago

  • Priority changed from normal to high

comment:6 in reply to: ↑ 4 ; follow-up: kurtpayne4 months ago

  • Cc kurtpayne added

I cannot reproduce this using the instructions given. I assume, based on the description, that there is an object cache present.

Replying to dd32:

So we've got 2 bugs here, based around the same thing,

  1. If wp_clear_scheduled_hook() has been run in another thread, wp_clear_scheduled_hook() will endlessly loop in the current thread, because:

Is there a reason that we can't just make crons uncacheable? Something like this (don't cringe)

add_filter( 'pre_option_crons', function() {
    wp_cache_delete( 'cron', 'options' );
    return false;
}, -PHP_INT_MAX );
  1. update_option() may now fail in "high-traffic" situations where an option is being updated my multiple threads to the same value.

Definitely can replicate this. If there is a multiple cache situation (like fastcgi + APC) then caches will get out of sync with each other and database very easily.

A unit test might be hard for this scenario, but should be able to replicate it by modifying the DB directly..

Yep. Let's focus on the cron issue, then move on to the bigger caching issue in another ticket. If you can help me reproduce this, I can start writing some tests around it.

dd324 months ago

comment:7 dd324 months ago

In 26782:

Cron: Fix a case where a cache inconsistency can cause wp_clear_scheduled_hook() to enter an infinite loop. This unravels the function from using other cron api functions to looping over the cron array directly. See #25773

comment:8 nacin4 months ago

Looks great to me. dimagsv, any feedback?

comment:9 nacin4 months ago

  • Keywords fixed-major added

comment:10 in reply to: ↑ 6 dimagsv4 months ago

Replying to kurtpayne:

I cannot reproduce this using the instructions given.

I have tried to reproduce this bug on an other hosting. Unsuccessfully. I think timing is important in this case. Special test plugin is needed to make this bug reproducible.

Replying to nacin:

Looks great to me. dimagsv, any feedback?

This will avoid hanging. Main issue will be fixed.

comment:11 dimagsv2 months ago

  • Resolution set to worksforme
  • Status changed from new to closed

comment:12 nacin2 months ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

Hi dimagsv, this ticket was fixed in 3.8, but we're leaving this ticket around in case we do a 3.7.2 release. Thanks!

comment:13 nacin3 weeks ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from reopened to closed

In 27886:

Cron: Fix a case where a cache inconsistency can cause wp_clear_scheduled_hook() to enter an infinite loop.

Merges [26782] from 3.8 to the 3.7 branch.

props dd32.
fixes #25773.

Note: See TracTickets for help on using tickets.