From e3d0a19872e67fc3b9f84f9ec4b47ca9d968a57b Mon Sep 17 00:00:00 2001 From: Erick Hitter Date: Tue, 1 Aug 2017 21:42:57 -0700 Subject: [PATCH 1/7] Tests for single-concurrency lock See #3 --- tests/test-lock.php | 58 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100755 tests/test-lock.php diff --git a/tests/test-lock.php b/tests/test-lock.php new file mode 100755 index 00000000..e8cb7f2e --- /dev/null +++ b/tests/test-lock.php @@ -0,0 +1,58 @@ +assertEquals( true, $should_be_free ); + + $should_be_locked = Cron_Control\Lock::check_lock( $lock_name, 1 ); + + $this->assertEquals( false, $should_be_locked ); + + Cron_Control\Lock::free_lock( $lock_name ); + + $should_be_free = Cron_Control\Lock::check_lock( $lock_name, 1 ); + + $this->assertEquals( true, $should_be_free ); + + Cron_Control\Lock::reset_lock( $lock_name ); + + $should_be_free = Cron_Control\Lock::check_lock( $lock_name, 1 ); + + $this->assertEquals( true, $should_be_free ); + } +} From 6b9a2997fb165be604d82151e72072850160660b Mon Sep 17 00:00:00 2001 From: Erick Hitter Date: Tue, 1 Aug 2017 21:44:16 -0700 Subject: [PATCH 2/7] Clarify --- tests/test-lock.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test-lock.php b/tests/test-lock.php index e8cb7f2e..725cfa0e 100755 --- a/tests/test-lock.php +++ b/tests/test-lock.php @@ -30,7 +30,7 @@ function tearDown() { /** * Test a single-concurrency lock */ - function test_single_lock() { + function test_single_concurrency_lock() { $lock_name = 'test-lock'; Cron_Control\Lock::prime_lock( $lock_name ); From a27d5721d208b8fda8af84f7ca22ac18256aa39c Mon Sep 17 00:00:00 2001 From: Erick Hitter Date: Tue, 1 Aug 2017 22:03:06 -0700 Subject: [PATCH 3/7] Test for multi-concurrency lock Fixes #3 --- tests/test-lock.php | 51 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 43 insertions(+), 8 deletions(-) diff --git a/tests/test-lock.php b/tests/test-lock.php index 725cfa0e..991d4bf1 100755 --- a/tests/test-lock.php +++ b/tests/test-lock.php @@ -32,27 +32,62 @@ function tearDown() { */ function test_single_concurrency_lock() { $lock_name = 'test-lock'; + $limit = 1; Cron_Control\Lock::prime_lock( $lock_name ); - $should_be_free = Cron_Control\Lock::check_lock( $lock_name, 1 ); + $can_run = Cron_Control\Lock::check_lock( $lock_name, $limit ); - $this->assertEquals( true, $should_be_free ); + $this->assertEquals( true, $can_run ); - $should_be_locked = Cron_Control\Lock::check_lock( $lock_name, 1 ); + $can_run = Cron_Control\Lock::check_lock( $lock_name, $limit ); - $this->assertEquals( false, $should_be_locked ); + $this->assertEquals( false, $can_run ); Cron_Control\Lock::free_lock( $lock_name ); - $should_be_free = Cron_Control\Lock::check_lock( $lock_name, 1 ); + $can_run = Cron_Control\Lock::check_lock( $lock_name, $limit ); - $this->assertEquals( true, $should_be_free ); + $this->assertEquals( true, $can_run ); Cron_Control\Lock::reset_lock( $lock_name ); - $should_be_free = Cron_Control\Lock::check_lock( $lock_name, 1 ); + $can_run = Cron_Control\Lock::check_lock( $lock_name, $limit ); - $this->assertEquals( true, $should_be_free ); + $this->assertEquals( true, $can_run ); + } + + /** + * Test a multiple-concurrency lock + */ + function test_multiple_concurrency_lock() { + $lock_name = 'test-lock'; + $limit = 5; + + Cron_Control\Lock::prime_lock( $lock_name ); + + $can_run = Cron_Control\Lock::check_lock( $lock_name, $limit ); + + $this->assertEquals( true, $can_run ); + $this->assertEquals( 1, Cron_Control\Lock::get_lock_value( $lock_name ) ); + + for ( $i = 0; $i < $limit; $i++ ) { + $can_run = Cron_Control\Lock::check_lock( $lock_name, $limit ); + } + + $this->assertEquals( false, $can_run ); + $this->assertEquals( $limit, Cron_Control\Lock::get_lock_value( $lock_name ) ); + + Cron_Control\Lock::free_lock( $lock_name ); + + $can_run = Cron_Control\Lock::check_lock( $lock_name, $limit ); + + $this->assertEquals( true, $can_run ); + + Cron_Control\Lock::reset_lock( $lock_name ); + + $can_run = Cron_Control\Lock::check_lock( $lock_name, $limit ); + + $this->assertEquals( true, $can_run ); } } From ff1ae81d7dd26b117cd9484ac30e6666558f6051 Mon Sep 17 00:00:00 2001 From: Erick Hitter Date: Tue, 1 Aug 2017 22:13:15 -0700 Subject: [PATCH 4/7] Abstract lock checking --- includes/class-lock.php | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/includes/class-lock.php b/includes/class-lock.php index ec8cf7cd..3fb6d401 100644 --- a/includes/class-lock.php +++ b/includes/class-lock.php @@ -29,6 +29,31 @@ public static function check_lock( $lock, $limit = null, $timeout = null ) { $limit = LOCK_DEFAULT_LIMIT; } + // Check lock according to limit + if ( 1 === $limit ) { + return self::check_single_lock( $lock ); + } else { + return self::check_multi_lock( $lock, $limit ); + } + } + + /** + * + */ + private static function check_single_lock( $lock ) { + // Check if process can run + if ( self::get_lock_value( $lock ) >= 1 ) { + return false; + } else { + wp_cache_incr( self::get_key( $lock ) ); + return true; + } + } + + /** + * + */ + private static function check_multi_lock( $lock, $limit ) { // Check if process can run if ( self::get_lock_value( $lock ) >= $limit ) { return false; From 2335e7d75a7d957d524e7d3a1c96edba2a1c4404 Mon Sep 17 00:00:00 2001 From: Erick Hitter Date: Tue, 1 Aug 2017 22:23:54 -0700 Subject: [PATCH 5/7] Abstract lock freeing --- includes/class-lock.php | 43 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 41 insertions(+), 2 deletions(-) diff --git a/includes/class-lock.php b/includes/class-lock.php index 3fb6d401..ae8affd3 100644 --- a/includes/class-lock.php +++ b/includes/class-lock.php @@ -38,10 +38,12 @@ public static function check_lock( $lock, $limit = null, $timeout = null ) { } /** + * Check a single-concurrency lock * + * @param string $lock + * @return bool */ private static function check_single_lock( $lock ) { - // Check if process can run if ( self::get_lock_value( $lock ) >= 1 ) { return false; } else { @@ -67,6 +69,25 @@ private static function check_multi_lock( $lock, $limit ) { * When event completes, allow another */ public static function free_lock( $lock, $expires = 0 ) { + $lock_value = self::get_lock_value( $lock ); + + if ( is_int( $lock_value ) ) { + self::free_single_lock( $lock, $expires ); + } else { + self::free_multi_lock( $lock, $expires ); + } + + return true; + } + + /** + * Free a single-concurrency lock + * + * @param string $lock + * @param int $expires + * @return bool + */ + private static function free_single_lock( $lock, $expires ) { if ( self::get_lock_value( $lock ) > 1 ) { wp_cache_decr( self::get_key( $lock ) ); } else { @@ -78,6 +99,13 @@ public static function free_lock( $lock, $expires = 0 ) { return true; } + /** + * + */ + private static function free_multi_lock( $lock, $expires ) { + return self::free_single_lock( $lock, $expires ); + } + /** * Build cache key */ @@ -109,7 +137,18 @@ public static function prime_lock( $lock, $expires = 0 ) { * Retrieve a lock from cache */ public static function get_lock_value( $lock ) { - return (int) wp_cache_get( self::get_key( $lock ), null, true ); + $value = wp_cache_get( self::get_key( $lock ), null, true ); + + if ( ! is_numeric( $value ) && ! is_array( $value ) ) { + self::reset_lock( $lock ); + return 0; + } + + if ( is_numeric( $value ) ) { + $value = (int) $value; + } + + return $value; } /** From 2ec681b8953affa20d01ae0969848091a892841f Mon Sep 17 00:00:00 2001 From: Erick Hitter Date: Tue, 1 Aug 2017 23:40:28 -0700 Subject: [PATCH 6/7] Implement methods abstracted in ff1ae81 and 2335e7d Needs better way of freeing the lock associated with a process. --- includes/class-lock.php | 74 ++++++++++++++++++++++++++++++++--------- tests/test-lock.php | 4 +-- 2 files changed, 60 insertions(+), 18 deletions(-) diff --git a/includes/class-lock.php b/includes/class-lock.php index ae8affd3..6e10a6f4 100644 --- a/includes/class-lock.php +++ b/includes/class-lock.php @@ -33,7 +33,7 @@ public static function check_lock( $lock, $limit = null, $timeout = null ) { if ( 1 === $limit ) { return self::check_single_lock( $lock ); } else { - return self::check_multi_lock( $lock, $limit ); + return self::check_multi_lock( $lock, $limit, $timeout ); } } @@ -53,16 +53,35 @@ private static function check_single_lock( $lock ) { } /** + * Check a multiple-concurrency lock * + * @param string $lock + * @param int $limit + * @param int $timeout + * @return bool */ - private static function check_multi_lock( $lock, $limit ) { - // Check if process can run - if ( self::get_lock_value( $lock ) >= $limit ) { - return false; + private static function check_multi_lock( $lock, $limit, $timeout ) { + $value = self::get_lock_value( $lock ); + + // Upgrade to timestamped multi-lock, otherwise clear deadlocks + if ( is_int( $value ) ) { + $value = array_fill( 0, $value, time() ); + } elseif ( is_array( $value ) ) { + $value = empty( $value ) ? array() : self::purge_stale_values( $value, $timeout ); } else { - wp_cache_incr( self::get_key( $lock ) ); - return true; + $value = array(); } + + // Still locked + if ( count( $value ) >= $limit ) { + return false; + } + + // Available, claim a slot + $value[] = time(); + wp_cache_set( self::get_key( $lock ), $value ); + + return true; } /** @@ -71,10 +90,15 @@ private static function check_multi_lock( $lock, $limit ) { public static function free_lock( $lock, $expires = 0 ) { $lock_value = self::get_lock_value( $lock ); + if ( empty( $lock_value ) ) { + wp_cache_set( self::get_key( $lock, 'timestamp' ), time(), null, $expires ); + return true; + } + if ( is_int( $lock_value ) ) { - self::free_single_lock( $lock, $expires ); + self::free_single_lock( $lock, $lock_value, $expires ); } else { - self::free_multi_lock( $lock, $expires ); + self::free_multi_lock( $lock, $lock_value, $expires ); } return true; @@ -84,26 +108,31 @@ public static function free_lock( $lock, $expires = 0 ) { * Free a single-concurrency lock * * @param string $lock + * @param mixed $lock_value * @param int $expires - * @return bool */ - private static function free_single_lock( $lock, $expires ) { - if ( self::get_lock_value( $lock ) > 1 ) { + private static function free_single_lock( $lock, $lock_value, $expires ) { + if ( $lock_value > 1 ) { wp_cache_decr( self::get_key( $lock ) ); } else { wp_cache_set( self::get_key( $lock ), 0, null, $expires ); } wp_cache_set( self::get_key( $lock, 'timestamp' ), time(), null, $expires ); - - return true; } /** + * Free old multi-concurrency lock * + * @param string $lock + * @param mixed $lock_value + * @param int $expires */ - private static function free_multi_lock( $lock, $expires ) { - return self::free_single_lock( $lock, $expires ); + private static function free_multi_lock( $lock, $lock_value, $expires ) { + sort( $lock_value, SORT_NUMERIC ); + array_shift( $lock_value ); + wp_cache_set( self::get_key( $lock ), $lock_value, null, $expires ); + wp_cache_set( self::get_key( $lock, 'timestamp' ), time(), null, $expires ); } /** @@ -167,4 +196,17 @@ public static function reset_lock( $lock, $expires = 0 ) { return true; } + + /** + * Remove stale lock entries + * + * @param array $locks + * @param int $timeout + * @return array + */ + private static function purge_stale_values( $locks, $timeout ) { + return array_filter( $locks, function( $lock ) use( $timeout ) { + return $lock > time() - $timeout; + } ); + } } diff --git a/tests/test-lock.php b/tests/test-lock.php index 991d4bf1..7ef3b0fb 100755 --- a/tests/test-lock.php +++ b/tests/test-lock.php @@ -69,14 +69,14 @@ function test_multiple_concurrency_lock() { $can_run = Cron_Control\Lock::check_lock( $lock_name, $limit ); $this->assertEquals( true, $can_run ); - $this->assertEquals( 1, Cron_Control\Lock::get_lock_value( $lock_name ) ); + $this->assertEquals( 1, count( Cron_Control\Lock::get_lock_value( $lock_name ) ) ); for ( $i = 0; $i < $limit; $i++ ) { $can_run = Cron_Control\Lock::check_lock( $lock_name, $limit ); } $this->assertEquals( false, $can_run ); - $this->assertEquals( $limit, Cron_Control\Lock::get_lock_value( $lock_name ) ); + $this->assertEquals( $limit, count( Cron_Control\Lock::get_lock_value( $lock_name ) ) ); Cron_Control\Lock::free_lock( $lock_name ); From 44c722f867a73ecbf7d971fa0c1924235f7d14c1 Mon Sep 17 00:00:00 2001 From: Erick Hitter Date: Tue, 1 Aug 2017 23:52:56 -0700 Subject: [PATCH 7/7] Cleanup --- includes/class-lock.php | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/includes/class-lock.php b/includes/class-lock.php index 6e10a6f4..8f0451cf 100644 --- a/includes/class-lock.php +++ b/includes/class-lock.php @@ -25,15 +25,15 @@ public static function check_lock( $lock, $limit = null, $timeout = null ) { } // Default limit for concurrent events - if ( ! is_numeric( $limit ) ) { + if ( ! is_int( $limit ) ) { $limit = LOCK_DEFAULT_LIMIT; } // Check lock according to limit if ( 1 === $limit ) { - return self::check_single_lock( $lock ); + return self::check_single_lock( $lock, self::get_lock_value( $lock ) ); } else { - return self::check_multi_lock( $lock, $limit, $timeout ); + return self::check_multi_lock( $lock, self::get_lock_value( $lock ), $limit, $timeout ); } } @@ -41,10 +41,11 @@ public static function check_lock( $lock, $limit = null, $timeout = null ) { * Check a single-concurrency lock * * @param string $lock + * @param int $lock_value * @return bool */ - private static function check_single_lock( $lock ) { - if ( self::get_lock_value( $lock ) >= 1 ) { + private static function check_single_lock( $lock, $lock_value ) { + if ( $lock_value >= 1 ) { return false; } else { wp_cache_incr( self::get_key( $lock ) ); @@ -56,30 +57,29 @@ private static function check_single_lock( $lock ) { * Check a multiple-concurrency lock * * @param string $lock + * @param mixed $lock_value * @param int $limit * @param int $timeout * @return bool */ - private static function check_multi_lock( $lock, $limit, $timeout ) { - $value = self::get_lock_value( $lock ); - + private static function check_multi_lock( $lock, $lock_value, $limit, $timeout ) { // Upgrade to timestamped multi-lock, otherwise clear deadlocks - if ( is_int( $value ) ) { - $value = array_fill( 0, $value, time() ); - } elseif ( is_array( $value ) ) { - $value = empty( $value ) ? array() : self::purge_stale_values( $value, $timeout ); + if ( is_int( $lock_value ) ) { + $lock_value = array_fill( 0, $lock_value, time() ); + } elseif ( is_array( $lock_value ) ) { + $lock_value = empty( $lock_value ) ? array() : self::purge_stale_values( $lock_value, $timeout ); } else { - $value = array(); + $lock_value = array(); } // Still locked - if ( count( $value ) >= $limit ) { + if ( count( $lock_value ) >= $limit ) { return false; } // Available, claim a slot - $value[] = time(); - wp_cache_set( self::get_key( $lock ), $value ); + $lock_value[] = time(); + wp_cache_set( self::get_key( $lock ), $lock_value ); return true; }