From b5c21ec61216c31b0385db7c80f814e3c8941955 Mon Sep 17 00:00:00 2001 From: barryhughes <3594411+barryhughes@users.noreply.github.com> Date: Thu, 29 Jun 2023 15:10:48 -0700 Subject: [PATCH 1/8] Describe how option locks should handle possible concurrent claims. --- .../lock/ActionScheduler_OptionLock_Test.php | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/phpunit/lock/ActionScheduler_OptionLock_Test.php b/tests/phpunit/lock/ActionScheduler_OptionLock_Test.php index 4ed6a23f9..146354121 100644 --- a/tests/phpunit/lock/ActionScheduler_OptionLock_Test.php +++ b/tests/phpunit/lock/ActionScheduler_OptionLock_Test.php @@ -42,4 +42,32 @@ public function test_get_expiration() { $this->assertGreaterThan( $current_time, $expiration ); $this->assertLessThan( $current_time + MINUTE_IN_SECONDS + 1, $expiration ); } + + /** + * A call to `ActionScheduler::lock()->set()` should fail if the lock is already held (ie, by another process). + * + * @return void + */ + public function test_lock_resists_race_conditions() { + $lock = ActionScheduler::lock(); + $type = md5( wp_rand() ); + + // Approximate conditions in which a concurrently executing request manages to set (and obtain) the lock + // immediately before the current request can do so. + $simulate_concurrent_claim = function ( $query ) use ( $lock, $type ) { + static $executed = false; + + if ( ! $executed && false !== strpos( $query, 'action_scheduler_lock_' ) && 0 === strpos( $query, 'UPDATE' ) ) { + $executed = true; + $lock->set( $type ); + } + + return $query; + }; + + add_filter( 'query', $simulate_concurrent_claim ); + $this->assertFalse( $lock->is_locked( $type ), 'Initially, the lock is not held' ); + $this->assertFalse( $lock->set( $type ), 'The lock was not obtained, because another process already claimed it.' ); + remove_filter( 'query', $simulate_concurrent_claim ); + } } From 636e2988e49e70517dc7ead4221b324db61e8dc3 Mon Sep 17 00:00:00 2001 From: barryhughes <3594411+barryhughes@users.noreply.github.com> Date: Thu, 29 Jun 2023 15:11:27 -0700 Subject: [PATCH 2/8] Update Option Lock implementation to reduce potential for race conditions. --- classes/ActionScheduler_OptionLock.php | 95 +++++++++++++++++++++++++- 1 file changed, 93 insertions(+), 2 deletions(-) diff --git a/classes/ActionScheduler_OptionLock.php b/classes/ActionScheduler_OptionLock.php index 4bc9a3fc2..f02900afe 100644 --- a/classes/ActionScheduler_OptionLock.php +++ b/classes/ActionScheduler_OptionLock.php @@ -24,7 +24,25 @@ class ActionScheduler_OptionLock extends ActionScheduler_Lock { * @bool True if lock value has changed, false if not or if set failed. */ public function set( $lock_type ) { - return update_option( $this->get_key( $lock_type ), time() + $this->get_duration( $lock_type ) ); + global $wpdb; + + $existing_lock_value = $this->get_or_create_lock( $lock_type ); + $expiration_timestamp = $this->get_expiration_from( $existing_lock_value ); + + // The lock is already held. + if ( $expiration_timestamp >= time() ) { + return false; + } + + // Otherwise, try to obtain it. + return (bool) $wpdb->update( + $wpdb->options, + array( 'option_value' => $this->new_lock_value( $lock_type ) ), + array( + 'option_name' => $this->get_key( $lock_type ), + 'option_value' => $existing_lock_value, + ) + ); } /** @@ -34,7 +52,30 @@ public function set( $lock_type ) { * @return bool|int False if no lock is set, otherwise the timestamp for when the lock is set to expire. */ public function get_expiration( $lock_type ) { - return get_option( $this->get_key( $lock_type ) ); + return $this->get_expiration_from( $this->get_or_create_lock( $lock_type ) ); + } + + /** + * Given the lock string, derives the lock expiration timestamp (or false if it cannot be determined). + * + * @param string $lock_value String containing a timestamp, or pipe-separated combination of unique value and timestamp. + * + * @return false|int + */ + private function get_expiration_from( $lock_value ) { + $lock_string = explode( '|', $lock_value ); + + // Old style lock? + if ( count( $lock_string ) === 1 && is_numeric( $lock_string[0] ) ) { + return (int) $lock_string[0]; + } + + // New style lock? + if ( count( $lock_string ) === 2 && is_numeric( $lock_string[1] ) ) { + return (int) $lock_string[1]; + } + + return false; } /** @@ -46,4 +87,54 @@ public function get_expiration( $lock_type ) { protected function get_key( $lock_type ) { return sprintf( 'action_scheduler_lock_%s', $lock_type ); } + + /** + * Supplies the existing lock value, or an empty string if not set. + * + * @param string $lock_type A string to identify different lock types. + * + * @return string + */ + private function get_or_create_lock( $lock_type ) { + global $wpdb; + $lock_key = $this->get_key( $lock_type ); + + // Try to ensure the lock option exists, creating it if it does not already exist. + $wpdb->query( + $wpdb->prepare( + " + INSERT INTO $wpdb->options ( option_name, option_value, autoload ) + SELECT %s, '', %s + WHERE NOT EXISTS ( SELECT 1 FROM $wpdb->options WHERE option_name = %s ) + ", + array( + $lock_key, + 'no', + $lock_key, + ) + ) + ); + + // Now grab the existing lock value. + return (string) $wpdb->get_var( + $wpdb->prepare( + "SELECT option_value FROM $wpdb->options WHERE option_name = %s", + $this->get_key( $lock_type ) + ) + ); + } + + /** + * Supplies a lock value consisting of a unique value and the current timestamp, which are separated by a pipe + * character. + * + * Example: (string) "649de012e6b262.09774912|1688068114" + * + * @param string $lock_type A string to identify different lock types. + * + * @return string + */ + private function new_lock_value( $lock_type ) { + return uniqid( '', true ) . '|' . ( time() + $this->get_duration( $lock_type ) ); + } } From d1a5342a3ff80b07e97432d3f652f8c61beb816f Mon Sep 17 00:00:00 2001 From: barryhughes <3594411+barryhughes@users.noreply.github.com> Date: Thu, 29 Jun 2023 15:12:08 -0700 Subject: [PATCH 3/8] Check that lock was set, before proceeding to dispatch async request. Note that we still check is_locked() because, although the option lock implementation has been updated in such a way that we can rely on just calling set(), we do not know that that is the active implementation. --- classes/ActionScheduler_QueueRunner.php | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/classes/ActionScheduler_QueueRunner.php b/classes/ActionScheduler_QueueRunner.php index 96925b296..1ec3eab2a 100644 --- a/classes/ActionScheduler_QueueRunner.php +++ b/classes/ActionScheduler_QueueRunner.php @@ -103,9 +103,12 @@ public function unhook_dispatch_async_request() { * should dispatch a request to process pending actions. */ public function maybe_dispatch_async_request() { - if ( is_admin() && ! ActionScheduler::lock()->is_locked( 'async-request-runner' ) ) { - // Only start an async queue at most once every 60 seconds - ActionScheduler::lock()->set( 'async-request-runner' ); + // Only start an async queue at most once every 60 seconds. + if ( + is_admin() + && ! ActionScheduler::lock()->is_locked( 'async-request-runner' ) + && ActionScheduler::lock()->set( 'async-request-runner' ) + ) { $this->async_request->maybe_dispatch(); } } From 530baa1d9379dec220f2612050fd497daef58975 Mon Sep 17 00:00:00 2001 From: barryhughes <3594411+barryhughes@users.noreply.github.com> Date: Thu, 29 Jun 2023 15:14:40 -0700 Subject: [PATCH 4/8] Add guidance for concrete lock::set implementations. --- classes/abstracts/ActionScheduler_Lock.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/classes/abstracts/ActionScheduler_Lock.php b/classes/abstracts/ActionScheduler_Lock.php index 86e852851..e388a58fa 100644 --- a/classes/abstracts/ActionScheduler_Lock.php +++ b/classes/abstracts/ActionScheduler_Lock.php @@ -26,6 +26,8 @@ public function is_locked( $lock_type ) { /** * Set a lock. * + * To prevent race conditions, implementations should avoid setting the lock if the lock is already held. + * * @param string $lock_type A string to identify different lock types. * @return bool */ From 895e8d0076590ec1fe55fb1fe52af65e3d4a8340 Mon Sep 17 00:00:00 2001 From: barryhughes <3594411+barryhughes@users.noreply.github.com> Date: Wed, 5 Jul 2023 07:52:10 -0700 Subject: [PATCH 5/8] Be efficient! Be lazy in terms of creating the lock option. --- classes/ActionScheduler_OptionLock.php | 57 ++++++++++++++------------ 1 file changed, 30 insertions(+), 27 deletions(-) diff --git a/classes/ActionScheduler_OptionLock.php b/classes/ActionScheduler_OptionLock.php index f02900afe..336cf68b9 100644 --- a/classes/ActionScheduler_OptionLock.php +++ b/classes/ActionScheduler_OptionLock.php @@ -26,20 +26,40 @@ class ActionScheduler_OptionLock extends ActionScheduler_Lock { public function set( $lock_type ) { global $wpdb; - $existing_lock_value = $this->get_or_create_lock( $lock_type ); - $expiration_timestamp = $this->get_expiration_from( $existing_lock_value ); + $lock_key = $this->get_key( $lock_type ); + $existing_lock_value = $this->get_existing_lock( $lock_type ); + $new_lock_value = $this->new_lock_value( $lock_type ); - // The lock is already held. - if ( $expiration_timestamp >= time() ) { + // The lock may not exist yet, or may have been deleted. + if ( empty( $existing_lock_value ) ) { + return (bool) $wpdb->query( + $wpdb->prepare( + " + INSERT INTO $wpdb->options ( 'option_name', 'option_value', 'autoload' ) + SELECT %s, %s, 'no' + WHERE NOT EXISTS ( + SELECT 1 + FROM $wpdb->options + WHERE option_name = %s + ) + ", + $lock_key, + $new_lock_value, + $lock_key + ) + ); + } + + if ( $this->get_expiration_from( $existing_lock_value ) >= time() ) { return false; } - // Otherwise, try to obtain it. + // Otherwise, try to obtain the lock. return (bool) $wpdb->update( $wpdb->options, - array( 'option_value' => $this->new_lock_value( $lock_type ) ), + array( 'option_value' => $new_lock_value ), array( - 'option_name' => $this->get_key( $lock_type ), + 'option_name' => $lock_key, 'option_value' => $existing_lock_value, ) ); @@ -52,7 +72,7 @@ public function set( $lock_type ) { * @return bool|int False if no lock is set, otherwise the timestamp for when the lock is set to expire. */ public function get_expiration( $lock_type ) { - return $this->get_expiration_from( $this->get_or_create_lock( $lock_type ) ); + return $this->get_expiration_from( $this->get_existing_lock( $lock_type ) ); } /** @@ -95,27 +115,10 @@ protected function get_key( $lock_type ) { * * @return string */ - private function get_or_create_lock( $lock_type ) { + private function get_existing_lock( $lock_type ) { global $wpdb; - $lock_key = $this->get_key( $lock_type ); - - // Try to ensure the lock option exists, creating it if it does not already exist. - $wpdb->query( - $wpdb->prepare( - " - INSERT INTO $wpdb->options ( option_name, option_value, autoload ) - SELECT %s, '', %s - WHERE NOT EXISTS ( SELECT 1 FROM $wpdb->options WHERE option_name = %s ) - ", - array( - $lock_key, - 'no', - $lock_key, - ) - ) - ); - // Now grab the existing lock value. + // Now grab the existing lock value, if there is one. return (string) $wpdb->get_var( $wpdb->prepare( "SELECT option_value FROM $wpdb->options WHERE option_name = %s", From 62f0d332afe8d9cf2c3f7078aeab0f54dfda431d Mon Sep 17 00:00:00 2001 From: barryhughes <3594411+barryhughes@users.noreply.github.com> Date: Wed, 5 Jul 2023 09:58:27 -0700 Subject: [PATCH 6/8] Unquote column names. --- classes/ActionScheduler_OptionLock.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/classes/ActionScheduler_OptionLock.php b/classes/ActionScheduler_OptionLock.php index 336cf68b9..035d27e42 100644 --- a/classes/ActionScheduler_OptionLock.php +++ b/classes/ActionScheduler_OptionLock.php @@ -35,7 +35,7 @@ public function set( $lock_type ) { return (bool) $wpdb->query( $wpdb->prepare( " - INSERT INTO $wpdb->options ( 'option_name', 'option_value', 'autoload' ) + INSERT INTO $wpdb->options ( option_name, option_value, autoload ) SELECT %s, %s, 'no' WHERE NOT EXISTS ( SELECT 1 From b215f727b65c8507b352a546964e0c396697607d Mon Sep 17 00:00:00 2001 From: barryhughes <3594411+barryhughes@users.noreply.github.com> Date: Fri, 7 Jul 2023 07:39:31 -0700 Subject: [PATCH 7/8] Simplify insert, remove explicit check for existing entry. At least as far back as WP 5.2 (the oldest version we support), the option_name column is defined as unique: therefore we can rely on MySQL to avoid duplicates. Of course, if the schema has been modified and the unique constraint removed, that would be problematic -- but it seems reasonable for us to assume that isn't the case. --- classes/ActionScheduler_OptionLock.php | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/classes/ActionScheduler_OptionLock.php b/classes/ActionScheduler_OptionLock.php index 035d27e42..f503c3c4e 100644 --- a/classes/ActionScheduler_OptionLock.php +++ b/classes/ActionScheduler_OptionLock.php @@ -32,20 +32,13 @@ public function set( $lock_type ) { // The lock may not exist yet, or may have been deleted. if ( empty( $existing_lock_value ) ) { - return (bool) $wpdb->query( - $wpdb->prepare( - " - INSERT INTO $wpdb->options ( option_name, option_value, autoload ) - SELECT %s, %s, 'no' - WHERE NOT EXISTS ( - SELECT 1 - FROM $wpdb->options - WHERE option_name = %s - ) - ", - $lock_key, - $new_lock_value, - $lock_key + + return (bool) $wpdb->insert( + $wpdb->options, + array( + 'option_name' => $lock_key, + 'option_value' => $new_lock_value, + 'autoload' => 'no', ) ); } From bd2d81a8fb06e0cd35327eb0838b28f40ee96958 Mon Sep 17 00:00:00 2001 From: barryhughes <3594411+barryhughes@users.noreply.github.com> Date: Fri, 7 Jul 2023 07:43:27 -0700 Subject: [PATCH 8/8] Cleanup. --- classes/ActionScheduler_OptionLock.php | 1 - 1 file changed, 1 deletion(-) diff --git a/classes/ActionScheduler_OptionLock.php b/classes/ActionScheduler_OptionLock.php index f503c3c4e..911f9b77c 100644 --- a/classes/ActionScheduler_OptionLock.php +++ b/classes/ActionScheduler_OptionLock.php @@ -32,7 +32,6 @@ public function set( $lock_type ) { // The lock may not exist yet, or may have been deleted. if ( empty( $existing_lock_value ) ) { - return (bool) $wpdb->insert( $wpdb->options, array(