From 57eef8a60563c5169724fedb3ef9441bc2e78590 Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Tue, 6 Feb 2018 16:59:04 -0800 Subject: [PATCH 01/12] Add FileBreakpointStorage and test --- .../FileBreakpointStorage.php | 96 +++++++++++++++++++ .../FileBreakpointStorageTest.php | 48 ++++++++++ 2 files changed, 144 insertions(+) create mode 100644 src/Debugger/BreakpointStorage/FileBreakpointStorage.php create mode 100644 tests/unit/Debugger/BreakpointStorage/FileBreakpointStorageTest.php diff --git a/src/Debugger/BreakpointStorage/FileBreakpointStorage.php b/src/Debugger/BreakpointStorage/FileBreakpointStorage.php new file mode 100644 index 000000000000..d11e4b0585f2 --- /dev/null +++ b/src/Debugger/BreakpointStorage/FileBreakpointStorage.php @@ -0,0 +1,96 @@ +filename = $filename ?: $this->defaultFilename(); + } + + /** + * Save the given debugger breakpoints. + * + * @param Debuggee $debuggee + * @param Breakpoint[] $breakpoints + * @return bool + * @throws \RuntimeException when failed to attach to the shared memory. + */ + public function save(Debuggee $debuggee, array $breakpoints) + { + $data = [ + 'debuggeeId' => $debuggee->id(), + 'breakpoints' => $breakpoints + ]; + + $fp = fopen($this->filename, 'r+'); + flock($fp, LOCK_EX); + $success = fwrite($fp, serialize($data)) !== false; + flock($fp, LOCK_UN); + fclose($fp); + return $success; + } + + /** + * Load debugger breakpoints from the storage. Returns a 2-arity array + * with the debuggee id and the list of breakpoints. + * + * @return array + * @throws \RuntimeException when failed to attach to the shared memory. + */ + public function load() + { + if (file_exists($this->filename)) { + $fp = fopen($this->filename, 'r'); + $contents = ''; + flock($fp, LOCK_SH); + $contents = stream_get_contents($fp); + flock($fp, LOCK_UN); + fclose($fp); + $data = unserialize($contents); + return [ + $data['debuggeeId'], $data['breakpoints'] + ]; + } else { + return []; + } + } + + private function defaultFilename() + { + $bt = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 3); + $frame = $bt[2]; + return tempnam(sys_get_temp_dir(), $frame['file'] . $frame['line']); + } +} diff --git a/tests/unit/Debugger/BreakpointStorage/FileBreakpointStorageTest.php b/tests/unit/Debugger/BreakpointStorage/FileBreakpointStorageTest.php new file mode 100644 index 000000000000..26bbf9164eed --- /dev/null +++ b/tests/unit/Debugger/BreakpointStorage/FileBreakpointStorageTest.php @@ -0,0 +1,48 @@ +storage = new FileBreakpointStorage(); + } + + public function testSaveAndLoad() + { + $connection = $this->prophesize(ConnectionInterface::class); + $debuggee = new Debuggee($connection->reveal(), ['id' => 'debuggee1', 'project' => 'project1']); + $breakpoints = [ + new Breakpoint(['id' => 'breakpoint1']) + ]; + $this->storage->save($debuggee, $breakpoints); + $this->assertEquals(['debuggee1', $breakpoints], $this->storage->load()); + } +} From 53e45b5312f0d390b2ccf51c5364a7de6f55e353 Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Tue, 6 Feb 2018 17:05:08 -0800 Subject: [PATCH 02/12] Agent and Daemon default storage will use FileBreakpointStorage if sysv unavailable --- src/Debugger/Agent.php | 7 ++++++- src/Debugger/Daemon.php | 13 ++++++++++++- .../BreakpointStorage/FileBreakpointStorageTest.php | 1 + .../BreakpointStorage/SysvBreakpointStorageTest.php | 2 +- 4 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/Debugger/Agent.php b/src/Debugger/Agent.php index 1d1bef8fff22..d277e4ff4adf 100644 --- a/src/Debugger/Agent.php +++ b/src/Debugger/Agent.php @@ -19,7 +19,9 @@ use Google\Cloud\Core\Batch\BatchRunner; use Google\Cloud\Core\Batch\BatchTrait; +use Google\Cloud\Core\SysvTrait; use Google\Cloud\Debugger\BreakpointStorage\BreakpointStorageInterface; +use Google\Cloud\Debugger\BreakpointStorage\FileBreakpointStorage; use Google\Cloud\Debugger\BreakpointStorage\SysvBreakpointStorage; use Google\Cloud\Logging\LoggingClient; use Psr\Log\LoggerInterface; @@ -39,6 +41,7 @@ class Agent { use BatchTrait; + use SysvTrait; /** * @var Debuggee @@ -190,7 +193,9 @@ protected function getCallback() private function defaultStorage() { - return new SysvBreakpointStorage(); + return $this->isSysvIPCLoaded() + ? new SysvBreakpointStorage() + : new FileBreakpointStorage(); } private function defaultDebuggee() diff --git a/src/Debugger/Daemon.php b/src/Debugger/Daemon.php index 326733ecb569..87f92775aad6 100644 --- a/src/Debugger/Daemon.php +++ b/src/Debugger/Daemon.php @@ -17,10 +17,12 @@ namespace Google\Cloud\Debugger; +use Google\Cloud\Core\SysvTrait; use Google\Cloud\Core\Report\MetadataProviderInterface; use Google\Cloud\Core\Report\MetadataProviderUtils; use Google\Cloud\Core\Exception\ConflictException; use Google\Cloud\Debugger\BreakpointStorage\BreakpointStorageInterface; +use Google\Cloud\Debugger\BreakpointStorage\FileBreakpointStorage; use Google\Cloud\Debugger\BreakpointStorage\SysvBreakpointStorage; /** @@ -39,6 +41,8 @@ */ class Daemon { + use SysvTrait; + /** * @var Debuggee */ @@ -122,7 +126,7 @@ public function __construct($sourceRoot, array $options = []) $this->storage = array_key_exists('storage', $options) ? $options['storage'] - : new SysvBreakpointStorage(); + : $this->defaultStorage(); } /** @@ -222,4 +226,11 @@ private function defaultLabels(MetadataProviderInterface $metadataProvider = nul } return $labels; } + + private function defaultStorage() + { + return $this->isSysvIPCLoaded() + ? new SysvBreakpointStorage() + : new FileBreakpointStorage(); + } } diff --git a/tests/unit/Debugger/BreakpointStorage/FileBreakpointStorageTest.php b/tests/unit/Debugger/BreakpointStorage/FileBreakpointStorageTest.php index 26bbf9164eed..f54c26156490 100644 --- a/tests/unit/Debugger/BreakpointStorage/FileBreakpointStorageTest.php +++ b/tests/unit/Debugger/BreakpointStorage/FileBreakpointStorageTest.php @@ -33,6 +33,7 @@ class FileBreakpointStorageTest extends TestCase public function setUp() { $this->storage = new FileBreakpointStorage(); + var_dump($this->storage); } public function testSaveAndLoad() diff --git a/tests/unit/Debugger/BreakpointStorage/SysvBreakpointStorageTest.php b/tests/unit/Debugger/BreakpointStorage/SysvBreakpointStorageTest.php index 59f0114de4e8..fc9220f92be6 100644 --- a/tests/unit/Debugger/BreakpointStorage/SysvBreakpointStorageTest.php +++ b/tests/unit/Debugger/BreakpointStorage/SysvBreakpointStorageTest.php @@ -35,7 +35,7 @@ class SysvBreakpointStorageTest extends TestCase public function setUp() { - if (! $this->isSysvIPCLOaded()) { + if (!$this->isSysvIPCLoaded()) { $this->markTestSkipped( 'Skipping because SystemV IPC extensions are not loaded'); } From 6b16389a78a4d4bb79648c1e3a693b86f2596fcf Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Tue, 6 Feb 2018 17:27:41 -0800 Subject: [PATCH 03/12] default filename should be repeatable. need to lock a separate file --- .../BreakpointStorage/FileBreakpointStorage.php | 11 ++++++----- .../BreakpointStorage/FileBreakpointStorageTest.php | 1 - 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Debugger/BreakpointStorage/FileBreakpointStorage.php b/src/Debugger/BreakpointStorage/FileBreakpointStorage.php index d11e4b0585f2..4ebbd1eff8a3 100644 --- a/src/Debugger/BreakpointStorage/FileBreakpointStorage.php +++ b/src/Debugger/BreakpointStorage/FileBreakpointStorage.php @@ -37,6 +37,7 @@ class FileBreakpointStorage implements BreakpointStorageInterface public function __construct($filename = null) { $this->filename = $filename ?: $this->defaultFilename(); + $this->lockFilename = $this->filename . '.lock'; } /** @@ -54,9 +55,9 @@ public function save(Debuggee $debuggee, array $breakpoints) 'breakpoints' => $breakpoints ]; - $fp = fopen($this->filename, 'r+'); + $fp = fopen($this->lockFilename, 'w+'); flock($fp, LOCK_EX); - $success = fwrite($fp, serialize($data)) !== false; + $success = file_put_contents($this->filename, serialize($data)) !== false; flock($fp, LOCK_UN); fclose($fp); return $success; @@ -72,10 +73,10 @@ public function save(Debuggee $debuggee, array $breakpoints) public function load() { if (file_exists($this->filename)) { - $fp = fopen($this->filename, 'r'); + $fp = fopen($this->lockFilename, 'w+'); $contents = ''; flock($fp, LOCK_SH); - $contents = stream_get_contents($fp); + $contents = file_get_contents($this->filename); flock($fp, LOCK_UN); fclose($fp); $data = unserialize($contents); @@ -91,6 +92,6 @@ private function defaultFilename() { $bt = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 3); $frame = $bt[2]; - return tempnam(sys_get_temp_dir(), $frame['file'] . $frame['line']); + return implode(DIRECTORY_SEPARATOR, [sys_get_temp_dir(), basename($frame['file']) . $frame['line']]); } } diff --git a/tests/unit/Debugger/BreakpointStorage/FileBreakpointStorageTest.php b/tests/unit/Debugger/BreakpointStorage/FileBreakpointStorageTest.php index f54c26156490..26bbf9164eed 100644 --- a/tests/unit/Debugger/BreakpointStorage/FileBreakpointStorageTest.php +++ b/tests/unit/Debugger/BreakpointStorage/FileBreakpointStorageTest.php @@ -33,7 +33,6 @@ class FileBreakpointStorageTest extends TestCase public function setUp() { $this->storage = new FileBreakpointStorage(); - var_dump($this->storage); } public function testSaveAndLoad() From 8b00dde2797ce7d3241a78678d449644caa6cfee Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Wed, 7 Feb 2018 10:01:36 -0800 Subject: [PATCH 04/12] Cannot base default filename off caller. The breakpoint storage location needs to be the same between the agent and daemon. --- .../BreakpointStorage/FileBreakpointStorage.php | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/Debugger/BreakpointStorage/FileBreakpointStorage.php b/src/Debugger/BreakpointStorage/FileBreakpointStorage.php index 4ebbd1eff8a3..5f9211948db7 100644 --- a/src/Debugger/BreakpointStorage/FileBreakpointStorage.php +++ b/src/Debugger/BreakpointStorage/FileBreakpointStorage.php @@ -25,6 +25,8 @@ */ class FileBreakpointStorage implements BreakpointStorageInterface { + const DEFAULT_FILENAME = 'debugger-breakpoints'; + /* @var string */ private $filename; @@ -36,7 +38,8 @@ class FileBreakpointStorage implements BreakpointStorageInterface */ public function __construct($filename = null) { - $this->filename = $filename ?: $this->defaultFilename(); + $filename = $filename ?: self::DEFAULT_FILENAME; + $this->filename = implode(DIRECTORY_SEPARATOR, [sys_get_temp_dir(), $filename]); $this->lockFilename = $this->filename . '.lock'; } @@ -87,11 +90,4 @@ public function load() return []; } } - - private function defaultFilename() - { - $bt = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 3); - $frame = $bt[2]; - return implode(DIRECTORY_SEPARATOR, [sys_get_temp_dir(), basename($frame['file']) . $frame['line']]); - } } From ea582fb99b2d9e305d897cf7eac21fafe562f395 Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Wed, 7 Feb 2018 10:13:04 -0800 Subject: [PATCH 05/12] Reading the breakpoints should use a non-block read lock. If the breakpoint file is being written to it's ok to skip breakpoints for this request. --- .../FileBreakpointStorage.php | 21 +++++++++---------- .../SysvBreakpointStorage.php | 2 +- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/src/Debugger/BreakpointStorage/FileBreakpointStorage.php b/src/Debugger/BreakpointStorage/FileBreakpointStorage.php index 5f9211948db7..bcf1c72694fd 100644 --- a/src/Debugger/BreakpointStorage/FileBreakpointStorage.php +++ b/src/Debugger/BreakpointStorage/FileBreakpointStorage.php @@ -71,23 +71,22 @@ public function save(Debuggee $debuggee, array $breakpoints) * with the debuggee id and the list of breakpoints. * * @return array - * @throws \RuntimeException when failed to attach to the shared memory. */ public function load() { if (file_exists($this->filename)) { $fp = fopen($this->lockFilename, 'w+'); $contents = ''; - flock($fp, LOCK_SH); - $contents = file_get_contents($this->filename); - flock($fp, LOCK_UN); - fclose($fp); - $data = unserialize($contents); - return [ - $data['debuggeeId'], $data['breakpoints'] - ]; - } else { - return []; + if (flock($fp, LOCK_SH | LOCK_NB)) { + $contents = file_get_contents($this->filename); + flock($fp, LOCK_UN); + fclose($fp); + $data = unserialize($contents); + return [ + $data['debuggeeId'], $data['breakpoints'] + ]; + } } + return [null, []]; } } diff --git a/src/Debugger/BreakpointStorage/SysvBreakpointStorage.php b/src/Debugger/BreakpointStorage/SysvBreakpointStorage.php index d88b9e1a3f3a..6822d24d8831 100644 --- a/src/Debugger/BreakpointStorage/SysvBreakpointStorage.php +++ b/src/Debugger/BreakpointStorage/SysvBreakpointStorage.php @@ -80,7 +80,7 @@ public function load() ); } if (!shm_has_var($shmid, self::VAR_KEY)) { - $result = []; + $result = [null, []]; } else { $result = shm_get_var($shmid, self::VAR_KEY); } From ac9afb0575f8d1ee48804e6d57b9075cee5862eb Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Wed, 7 Feb 2018 10:27:07 -0800 Subject: [PATCH 06/12] Add a few comments to the locking code --- .../FileBreakpointStorage.php | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/src/Debugger/BreakpointStorage/FileBreakpointStorage.php b/src/Debugger/BreakpointStorage/FileBreakpointStorage.php index bcf1c72694fd..19b02dfeaad3 100644 --- a/src/Debugger/BreakpointStorage/FileBreakpointStorage.php +++ b/src/Debugger/BreakpointStorage/FileBreakpointStorage.php @@ -34,7 +34,7 @@ class FileBreakpointStorage implements BreakpointStorageInterface * Create a new FileBreakpointStorage instance. * * @param string $filename [optional] The full path to the storage file. - * **Defaults to** a temporary file generated by the caller. + * **Defaults to** a temporary file in the system's temp directory. */ public function __construct($filename = null) { @@ -49,7 +49,6 @@ public function __construct($filename = null) * @param Debuggee $debuggee * @param Breakpoint[] $breakpoints * @return bool - * @throws \RuntimeException when failed to attach to the shared memory. */ public function save(Debuggee $debuggee, array $breakpoints) { @@ -58,10 +57,15 @@ public function save(Debuggee $debuggee, array $breakpoints) 'breakpoints' => $breakpoints ]; + $success = false; + + // Acquire an exclusive write lock (blocking). There should only be a + // single Daemon that can call this. $fp = fopen($this->lockFilename, 'w+'); - flock($fp, LOCK_EX); - $success = file_put_contents($this->filename, serialize($data)) !== false; - flock($fp, LOCK_UN); + if (flock($fp, LOCK_EX)) { + $success = file_put_contents($this->filename, serialize($data)) !== false; + flock($fp, LOCK_UN); + } fclose($fp); return $success; } @@ -75,8 +79,12 @@ public function save(Debuggee $debuggee, array $breakpoints) public function load() { if (file_exists($this->filename)) { - $fp = fopen($this->lockFilename, 'w+'); $contents = ''; + + // Acquire a read lock (non-blocking). If we fail (file is locked + // for writing), then we return an empty list of breakpoints and + // skip debugging for this request. + $fp = fopen($this->lockFilename, 'w+'); if (flock($fp, LOCK_SH | LOCK_NB)) { $contents = file_get_contents($this->filename); flock($fp, LOCK_UN); From 38e028a3329881b9da1e4c12ee82562185a8353c Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Wed, 7 Feb 2018 11:39:27 -0800 Subject: [PATCH 07/12] Use Core\FlockLock in FileBreakpointStorage. Add options to LockInterface#acquire for blocking and exclusive locking options. --- src/Core/Lock/FlockLock.php | 25 ++++++++- src/Core/Lock/LockInterface.php | 12 ++++- src/Core/Lock/LockTrait.php | 12 ++++- src/Core/Lock/SemaphoreLock.php | 21 +++++++- src/Core/Lock/SymfonyLockAdapter.php | 23 ++++++-- .../FileBreakpointStorage.php | 52 ++++++++++++------- 6 files changed, 115 insertions(+), 30 deletions(-) diff --git a/src/Core/Lock/FlockLock.php b/src/Core/Lock/FlockLock.php index 28cf566c1343..a358ecb7e361 100644 --- a/src/Core/Lock/FlockLock.php +++ b/src/Core/Lock/FlockLock.php @@ -62,10 +62,18 @@ public function __construct($fileName) /** * Acquires a lock that will block until released. * + * @param array $options [optional] { + * Configuration options. + * + * @type bool $blocking Whether the process should block while waiting + * to acquire the lock. **Defaults to** true. + * @type bool $exclusive If true, acquire an excluse (write) lock. If + * false, acquire a shared (read) lock. **Defaults to** true. + * } * @return bool * @throws \RuntimeException If the lock fails to be acquired. */ - public function acquire() + public function acquire(array $options = []) { if ($this->handle) { return true; @@ -73,7 +81,7 @@ public function acquire() $this->handle = $this->initializeHandle(); - if (!flock($this->handle, LOCK_EX)) { + if (!flock($this->handle, $this->lockType($options))) { fclose($this->handle); $this->handle = null; @@ -117,4 +125,17 @@ private function initializeHandle() return $handle; } + + private function lockType(array $options) + { + $options += [ + 'blocking' => true, + 'exclusive' => true + ]; + $lockType = $options['exclusive'] ? LOCK_EX : LOCK_SH; + if (!$options['blocking']) { + $lockType = $lockType | LOCK_UN; + } + return $lockType; + } } diff --git a/src/Core/Lock/LockInterface.php b/src/Core/Lock/LockInterface.php index b328de036a94..3b60d3bf4a38 100644 --- a/src/Core/Lock/LockInterface.php +++ b/src/Core/Lock/LockInterface.php @@ -25,10 +25,18 @@ interface LockInterface /** * Acquires a lock that will block until released. * + * @param array $options [optional] { + * Configuration options. + * + * @type bool $blocking Whether the process should block while waiting + * to acquire the lock. **Defaults to** true. + * @type bool $exclusive If true, acquire an excluse (write) lock. If + * false, acquire a shared (read) lock. **Defaults to** true. + * } * @return bool - * @throws \RuntimeException + * @throws \RuntimeException If the lock fails to be acquired. */ - public function acquire(); + public function acquire(array $options = []); /** * Releases the lock. diff --git a/src/Core/Lock/LockTrait.php b/src/Core/Lock/LockTrait.php index e20a0fda9f31..17539f948c9d 100644 --- a/src/Core/Lock/LockTrait.php +++ b/src/Core/Lock/LockTrait.php @@ -25,10 +25,18 @@ trait LockTrait /** * Acquires a lock that will block until released. * + * @param array $options [optional] { + * Configuration options. + * + * @type bool $blocking Whether the process should block while waiting + * to acquire the lock. **Defaults to** true. + * @type bool $exclusive If true, acquire an excluse (write) lock. If + * false, acquire a shared (read) lock. **Defaults to** true. + * } * @return bool - * @throws \RuntimeException + * @throws \RuntimeException If the lock fails to be acquired. */ - abstract public function acquire(); + abstract public function acquire(array $options = []); /** * Releases the lock. diff --git a/src/Core/Lock/SemaphoreLock.php b/src/Core/Lock/SemaphoreLock.php index c960cc9b09a2..678a213ae44e 100644 --- a/src/Core/Lock/SemaphoreLock.php +++ b/src/Core/Lock/SemaphoreLock.php @@ -64,18 +64,35 @@ public function __construct($key) /** * Acquires a lock that will block until released. * + * @param array $options [optional] { + * Configuration options. + * + * @type bool $blocking Whether the process should block while waiting + * to acquire the lock. **Defaults to** true. + * @type bool $exclusive If true, acquire an excluse (write) lock. If + * false, acquire a shared (read) lock. **Defaults to** true. + * } * @return bool * @throws \RuntimeException If the lock fails to be acquired. */ - public function acquire() + public function acquire(array $options = []) { + $options += [ + 'blocking' => true, + 'exclusive' => true + ]; + + if (!$options['exclusive']) { + trigger_error('SemaphoreLock does not support shared locking.', E_USER_WARNING); + } + if ($this->semaphoreId) { return true; } $this->semaphoreId = $this->initializeId(); - if (!sem_acquire($this->semaphoreId)) { + if (!sem_acquire($this->semaphoreId, !$options['blocking'])) { $this->semaphoreId = null; throw new \RuntimeException('Failed to acquire lock.'); diff --git a/src/Core/Lock/SymfonyLockAdapter.php b/src/Core/Lock/SymfonyLockAdapter.php index 2fc761a9eab7..4b787e783890 100644 --- a/src/Core/Lock/SymfonyLockAdapter.php +++ b/src/Core/Lock/SymfonyLockAdapter.php @@ -42,13 +42,30 @@ public function __construct(SymfonyLockInterface $lock) /** * Acquires a lock that will block until released. * + * @param array $options [optional] { + * Configuration options. + * + * @type bool $blocking Whether the process should block while waiting + * to acquire the lock. **Defaults to** true. + * @type bool $exclusive If true, acquire an excluse (write) lock. If + * false, acquire a shared (read) lock. **Defaults to** true. + * } * @return bool - * @throws \RuntimeException + * @throws \RuntimeException If the lock fails to be acquired. */ - public function acquire() + public function acquire(array $options = []) { + $options += [ + 'blocking' => true, + 'exclusive' => true + ]; + + if (!$options['exclusive']) { + trigger_error('SymfonyLockAdapter does not support shared locking.', E_USER_WARNING); + } + try { - return $this->lock->acquire(true); + return $this->lock->acquire($options['blocking']); } catch (\Exception $ex) { throw new \RuntimeException($ex->getMessage()); } diff --git a/src/Debugger/BreakpointStorage/FileBreakpointStorage.php b/src/Debugger/BreakpointStorage/FileBreakpointStorage.php index 19b02dfeaad3..7ad29cba240d 100644 --- a/src/Debugger/BreakpointStorage/FileBreakpointStorage.php +++ b/src/Debugger/BreakpointStorage/FileBreakpointStorage.php @@ -17,6 +17,8 @@ namespace Google\Cloud\Debugger\BreakpointStorage; +use Google\Cloud\Core\Lock\FlockLock; +use Google\Cloud\Core\Lock\LockInterface; use Google\Cloud\Debugger\Breakpoint; use Google\Cloud\Debugger\Debuggee; @@ -30,6 +32,9 @@ class FileBreakpointStorage implements BreakpointStorageInterface /* @var string */ private $filename; + /* @var LockInterface */ + private $lock; + /** * Create a new FileBreakpointStorage instance. * @@ -40,7 +45,7 @@ public function __construct($filename = null) { $filename = $filename ?: self::DEFAULT_FILENAME; $this->filename = implode(DIRECTORY_SEPARATOR, [sys_get_temp_dir(), $filename]); - $this->lockFilename = $this->filename . '.lock'; + $this->lock = new FlockLock($filename . '.lock'); } /** @@ -61,12 +66,16 @@ public function save(Debuggee $debuggee, array $breakpoints) // Acquire an exclusive write lock (blocking). There should only be a // single Daemon that can call this. - $fp = fopen($this->lockFilename, 'w+'); - if (flock($fp, LOCK_EX)) { - $success = file_put_contents($this->filename, serialize($data)) !== false; - flock($fp, LOCK_UN); + try { + $this->lock->acquire(); + try { + $success = file_put_contents($this->filename, serialize($data)) !== false; + } finally { + $this->lock->release(); + } + } catch (\RuntimeException $e) { + // Do nothing } - fclose($fp); return $success; } @@ -78,23 +87,28 @@ public function save(Debuggee $debuggee, array $breakpoints) */ public function load() { - if (file_exists($this->filename)) { - $contents = ''; + $debuggeeId = null; + $breakpoints = []; - // Acquire a read lock (non-blocking). If we fail (file is locked - // for writing), then we return an empty list of breakpoints and - // skip debugging for this request. - $fp = fopen($this->lockFilename, 'w+'); - if (flock($fp, LOCK_SH | LOCK_NB)) { + // Acquire a read lock (non-blocking). If we fail (file is locked + // for writing), then we return an empty list of breakpoints and + // skip debugging for this request. + try { + $this->lock->acquire([ + 'blocking' => false, + 'exclusive' => false + ]); + try { $contents = file_get_contents($this->filename); - flock($fp, LOCK_UN); - fclose($fp); $data = unserialize($contents); - return [ - $data['debuggeeId'], $data['breakpoints'] - ]; + $debuggeeId = $data['debuggeeId']; + $breakpoints = $data['breakpoints']; + } finally { + $this->lock->release(); } + } catch (\RuntimeException $e) { + // Do nothing } - return [null, []]; + return [$debuggeeId, $breakpoints]; } } From 06001a88710316d6a7984823d49355ee4be83d2d Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Thu, 8 Feb 2018 09:11:44 -0800 Subject: [PATCH 08/12] Use `LockTrait::synchronize()`. Address other PR feedback --- src/Core/Lock/LockInterface.php | 12 ++++++++++-- src/Core/Lock/LockTrait.php | 16 +++++++++++----- .../BreakpointStorage/FileBreakpointStorage.php | 9 +++------ .../FileBreakpointStorageTest.php | 2 +- 4 files changed, 25 insertions(+), 14 deletions(-) diff --git a/src/Core/Lock/LockInterface.php b/src/Core/Lock/LockInterface.php index 3b60d3bf4a38..06c20220ee8d 100644 --- a/src/Core/Lock/LockInterface.php +++ b/src/Core/Lock/LockInterface.php @@ -23,7 +23,7 @@ interface LockInterface { /** - * Acquires a lock that will block until released. + * Acquires a lock. * * @param array $options [optional] { * Configuration options. @@ -49,7 +49,15 @@ public function release(); * Execute a callable within a lock. * * @param callable $func The callable to execute. + * @param array $options [optional] { + * Configuration options. + * + * @type bool $blocking Whether the process should block while waiting + * to acquire the lock. **Defaults to** true. + * @type bool $exclusive If true, acquire an excluse (write) lock. If + * false, acquire a shared (read) lock. **Defaults to** true. + * } * @return mixed */ - public function synchronize(callable $func); + public function synchronize(callable $func, array $options = []); } diff --git a/src/Core/Lock/LockTrait.php b/src/Core/Lock/LockTrait.php index 17539f948c9d..dd24c908f058 100644 --- a/src/Core/Lock/LockTrait.php +++ b/src/Core/Lock/LockTrait.php @@ -46,19 +46,25 @@ abstract public function acquire(array $options = []); abstract public function release(); /** - * Execute a callable within a lock. If an exception is caught during - * execution of the callable the lock will first be released before throwing - * it. + * Execute a callable within a lock. * * @param callable $func The callable to execute. + * @param array $options [optional] { + * Configuration options. + * + * @type bool $blocking Whether the process should block while waiting + * to acquire the lock. **Defaults to** true. + * @type bool $exclusive If true, acquire an excluse (write) lock. If + * false, acquire a shared (read) lock. **Defaults to** true. + * } * @return mixed */ - public function synchronize(callable $func) + public function synchronize(callable $func, array $options = []) { $result = null; $exception = null; - if ($this->acquire()) { + if ($this->acquire($options)) { try { $result = $func(); } catch (\Exception $ex) { diff --git a/src/Debugger/BreakpointStorage/FileBreakpointStorage.php b/src/Debugger/BreakpointStorage/FileBreakpointStorage.php index 7ad29cba240d..a6364a881fc7 100644 --- a/src/Debugger/BreakpointStorage/FileBreakpointStorage.php +++ b/src/Debugger/BreakpointStorage/FileBreakpointStorage.php @@ -67,12 +67,9 @@ public function save(Debuggee $debuggee, array $breakpoints) // Acquire an exclusive write lock (blocking). There should only be a // single Daemon that can call this. try { - $this->lock->acquire(); - try { - $success = file_put_contents($this->filename, serialize($data)) !== false; - } finally { - $this->lock->release(); - } + $success = $this->lock->synchronize(function () use ($data) { + return file_put_contents($this->filename, serialize($data)) !== false; + }); } catch (\RuntimeException $e) { // Do nothing } diff --git a/tests/unit/Debugger/BreakpointStorage/FileBreakpointStorageTest.php b/tests/unit/Debugger/BreakpointStorage/FileBreakpointStorageTest.php index 26bbf9164eed..44fc726eedeb 100644 --- a/tests/unit/Debugger/BreakpointStorage/FileBreakpointStorageTest.php +++ b/tests/unit/Debugger/BreakpointStorage/FileBreakpointStorageTest.php @@ -1,6 +1,6 @@ Date: Thu, 8 Feb 2018 09:50:21 -0800 Subject: [PATCH 09/12] Move exclusive option to FlockLock constructor --- src/Core/Lock/FlockLock.php | 25 ++++++++++---- src/Core/Lock/LockInterface.php | 2 -- src/Core/Lock/LockTrait.php | 4 --- src/Core/Lock/SemaphoreLock.php | 9 +---- src/Core/Lock/SymfonyLockAdapter.php | 9 +---- .../FileBreakpointStorage.php | 34 ++++++++++--------- 6 files changed, 39 insertions(+), 44 deletions(-) diff --git a/src/Core/Lock/FlockLock.php b/src/Core/Lock/FlockLock.php index a358ecb7e361..eedfbb3f17b5 100644 --- a/src/Core/Lock/FlockLock.php +++ b/src/Core/Lock/FlockLock.php @@ -42,16 +42,32 @@ class FlockLock implements LockInterface */ private $handle; + /** + * @var bool If true, we should acquire an exclusive lock. + */ + private $exclusive; + /** * @param string $fileName The name of the file to use as a lock. + * @param array $options [optional] { + * Configuration options. + * + * @type bool $exclusive If true, acquire an excluse (write) lock. If + * false, acquire a shared (read) lock. **Defaults to** true. + * } * @throws \InvalidArgumentException If an invalid fileName is provided. */ - public function __construct($fileName) + public function __construct($fileName, array $options = []) { if (!is_string($fileName)) { throw new \InvalidArgumentException('$fileName must be a string.'); } + $options += [ + 'exclusive' => true + ]; + $this->exclusive = $options['exclusive']; + $this->filePath = sprintf( self::FILE_PATH_TEMPLATE, sys_get_temp_dir(), @@ -67,8 +83,6 @@ public function __construct($fileName) * * @type bool $blocking Whether the process should block while waiting * to acquire the lock. **Defaults to** true. - * @type bool $exclusive If true, acquire an excluse (write) lock. If - * false, acquire a shared (read) lock. **Defaults to** true. * } * @return bool * @throws \RuntimeException If the lock fails to be acquired. @@ -129,10 +143,9 @@ private function initializeHandle() private function lockType(array $options) { $options += [ - 'blocking' => true, - 'exclusive' => true + 'blocking' => true ]; - $lockType = $options['exclusive'] ? LOCK_EX : LOCK_SH; + $lockType = $this->exclusive ? LOCK_EX : LOCK_SH; if (!$options['blocking']) { $lockType = $lockType | LOCK_UN; } diff --git a/src/Core/Lock/LockInterface.php b/src/Core/Lock/LockInterface.php index 06c20220ee8d..d12156d49cab 100644 --- a/src/Core/Lock/LockInterface.php +++ b/src/Core/Lock/LockInterface.php @@ -30,8 +30,6 @@ interface LockInterface * * @type bool $blocking Whether the process should block while waiting * to acquire the lock. **Defaults to** true. - * @type bool $exclusive If true, acquire an excluse (write) lock. If - * false, acquire a shared (read) lock. **Defaults to** true. * } * @return bool * @throws \RuntimeException If the lock fails to be acquired. diff --git a/src/Core/Lock/LockTrait.php b/src/Core/Lock/LockTrait.php index dd24c908f058..2cfaebaa60b4 100644 --- a/src/Core/Lock/LockTrait.php +++ b/src/Core/Lock/LockTrait.php @@ -30,8 +30,6 @@ trait LockTrait * * @type bool $blocking Whether the process should block while waiting * to acquire the lock. **Defaults to** true. - * @type bool $exclusive If true, acquire an excluse (write) lock. If - * false, acquire a shared (read) lock. **Defaults to** true. * } * @return bool * @throws \RuntimeException If the lock fails to be acquired. @@ -54,8 +52,6 @@ abstract public function release(); * * @type bool $blocking Whether the process should block while waiting * to acquire the lock. **Defaults to** true. - * @type bool $exclusive If true, acquire an excluse (write) lock. If - * false, acquire a shared (read) lock. **Defaults to** true. * } * @return mixed */ diff --git a/src/Core/Lock/SemaphoreLock.php b/src/Core/Lock/SemaphoreLock.php index 678a213ae44e..f97d4b209df8 100644 --- a/src/Core/Lock/SemaphoreLock.php +++ b/src/Core/Lock/SemaphoreLock.php @@ -69,8 +69,6 @@ public function __construct($key) * * @type bool $blocking Whether the process should block while waiting * to acquire the lock. **Defaults to** true. - * @type bool $exclusive If true, acquire an excluse (write) lock. If - * false, acquire a shared (read) lock. **Defaults to** true. * } * @return bool * @throws \RuntimeException If the lock fails to be acquired. @@ -78,14 +76,9 @@ public function __construct($key) public function acquire(array $options = []) { $options += [ - 'blocking' => true, - 'exclusive' => true + 'blocking' => true ]; - if (!$options['exclusive']) { - trigger_error('SemaphoreLock does not support shared locking.', E_USER_WARNING); - } - if ($this->semaphoreId) { return true; } diff --git a/src/Core/Lock/SymfonyLockAdapter.php b/src/Core/Lock/SymfonyLockAdapter.php index 4b787e783890..7736b8bb9a2d 100644 --- a/src/Core/Lock/SymfonyLockAdapter.php +++ b/src/Core/Lock/SymfonyLockAdapter.php @@ -47,8 +47,6 @@ public function __construct(SymfonyLockInterface $lock) * * @type bool $blocking Whether the process should block while waiting * to acquire the lock. **Defaults to** true. - * @type bool $exclusive If true, acquire an excluse (write) lock. If - * false, acquire a shared (read) lock. **Defaults to** true. * } * @return bool * @throws \RuntimeException If the lock fails to be acquired. @@ -56,14 +54,9 @@ public function __construct(SymfonyLockInterface $lock) public function acquire(array $options = []) { $options += [ - 'blocking' => true, - 'exclusive' => true + 'blocking' => true ]; - if (!$options['exclusive']) { - trigger_error('SymfonyLockAdapter does not support shared locking.', E_USER_WARNING); - } - try { return $this->lock->acquire($options['blocking']); } catch (\Exception $ex) { diff --git a/src/Debugger/BreakpointStorage/FileBreakpointStorage.php b/src/Debugger/BreakpointStorage/FileBreakpointStorage.php index a6364a881fc7..8c6d1dafd892 100644 --- a/src/Debugger/BreakpointStorage/FileBreakpointStorage.php +++ b/src/Debugger/BreakpointStorage/FileBreakpointStorage.php @@ -18,7 +18,6 @@ namespace Google\Cloud\Debugger\BreakpointStorage; use Google\Cloud\Core\Lock\FlockLock; -use Google\Cloud\Core\Lock\LockInterface; use Google\Cloud\Debugger\Breakpoint; use Google\Cloud\Debugger\Debuggee; @@ -32,8 +31,8 @@ class FileBreakpointStorage implements BreakpointStorageInterface /* @var string */ private $filename; - /* @var LockInterface */ - private $lock; + /* @var string */ + private $lockFilename; /** * Create a new FileBreakpointStorage instance. @@ -45,7 +44,7 @@ public function __construct($filename = null) { $filename = $filename ?: self::DEFAULT_FILENAME; $this->filename = implode(DIRECTORY_SEPARATOR, [sys_get_temp_dir(), $filename]); - $this->lock = new FlockLock($filename . '.lock'); + $this->lockFilename = $filename . '.lock'; } /** @@ -67,7 +66,7 @@ public function save(Debuggee $debuggee, array $breakpoints) // Acquire an exclusive write lock (blocking). There should only be a // single Daemon that can call this. try { - $success = $this->lock->synchronize(function () use ($data) { + $success = $this->getLock(true)->synchronize(function () use ($data) { return file_put_contents($this->filename, serialize($data)) !== false; }); } catch (\RuntimeException $e) { @@ -91,21 +90,24 @@ public function load() // for writing), then we return an empty list of breakpoints and // skip debugging for this request. try { - $this->lock->acquire([ - 'blocking' => false, - 'exclusive' => false + $contents = $this->getLock()->synchronize(function() { + return file_get_contents($this->filename); + }, [ + 'blocking' => false ]); - try { - $contents = file_get_contents($this->filename); - $data = unserialize($contents); - $debuggeeId = $data['debuggeeId']; - $breakpoints = $data['breakpoints']; - } finally { - $this->lock->release(); - } + $data = unserialize($contents); + $debuggeeId = $data['debuggeeId']; + $breakpoints = $data['breakpoints']; } catch (\RuntimeException $e) { // Do nothing } return [$debuggeeId, $breakpoints]; } + + private function getLock($exclusive = false) + { + return new FlockLock($this->lockFilename, [ + 'exclusive' => $exclusive + ]); + } } From 3088f5b30871fe3da869deb2506d6918c4869ca7 Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Thu, 8 Feb 2018 09:53:44 -0800 Subject: [PATCH 10/12] Put LockTrait synchronize() description back --- src/Core/Lock/LockTrait.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Core/Lock/LockTrait.php b/src/Core/Lock/LockTrait.php index 2cfaebaa60b4..dfa5add3a7bd 100644 --- a/src/Core/Lock/LockTrait.php +++ b/src/Core/Lock/LockTrait.php @@ -44,7 +44,9 @@ abstract public function acquire(array $options = []); abstract public function release(); /** - * Execute a callable within a lock. + * Execute a callable within a lock. If an exception is caught during + * execution of the callable the lock will first be released before throwing + * it. * * @param callable $func The callable to execute. * @param array $options [optional] { From cd0072fd61ddf8430d5339308f894e9f3066cc4b Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Thu, 8 Feb 2018 09:59:49 -0800 Subject: [PATCH 11/12] Remove exclusive option from docs for LockInterface --- src/Core/Lock/LockInterface.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Core/Lock/LockInterface.php b/src/Core/Lock/LockInterface.php index d12156d49cab..1039362cfeae 100644 --- a/src/Core/Lock/LockInterface.php +++ b/src/Core/Lock/LockInterface.php @@ -52,8 +52,6 @@ public function release(); * * @type bool $blocking Whether the process should block while waiting * to acquire the lock. **Defaults to** true. - * @type bool $exclusive If true, acquire an excluse (write) lock. If - * false, acquire a shared (read) lock. **Defaults to** true. * } * @return mixed */ From 988c369d33c65fd4be7762a976abf3538385c6f2 Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Thu, 8 Feb 2018 10:46:06 -0800 Subject: [PATCH 12/12] phpcs fix --- src/Debugger/BreakpointStorage/FileBreakpointStorage.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Debugger/BreakpointStorage/FileBreakpointStorage.php b/src/Debugger/BreakpointStorage/FileBreakpointStorage.php index 8c6d1dafd892..03428675a5bb 100644 --- a/src/Debugger/BreakpointStorage/FileBreakpointStorage.php +++ b/src/Debugger/BreakpointStorage/FileBreakpointStorage.php @@ -90,7 +90,7 @@ public function load() // for writing), then we return an empty list of breakpoints and // skip debugging for this request. try { - $contents = $this->getLock()->synchronize(function() { + $contents = $this->getLock()->synchronize(function () { return file_get_contents($this->filename); }, [ 'blocking' => false