diff --git a/composer.json b/composer.json index cda417e5ea2b..5c8bcbb6e82d 100644 --- a/composer.json +++ b/composer.json @@ -81,8 +81,7 @@ }, "suggest": { "google/gax": "Required to support gRPC", - "google/proto-client-php": "Required to support gRPC", - "symfony/lock": "Required for the Spanner cached based session pool. Please require the following commit: 3.3.x-dev#1ba6ac9", + "google/proto-client": "Required to support gRPC", "phpseclib/phpseclib": "May be used in place of OpenSSL for creating signed Cloud Storage URLs. Please require version ^2." }, "autoload": { @@ -115,4 +114,4 @@ "entry": "ServiceBuilder.php" } } -} \ No newline at end of file +} diff --git a/src/Core/Batch/BatchDaemon.php b/src/Core/Batch/BatchDaemon.php index 31833ad7917a..c49952fdbe41 100644 --- a/src/Core/Batch/BatchDaemon.php +++ b/src/Core/Batch/BatchDaemon.php @@ -17,6 +17,8 @@ namespace Google\Cloud\Core\Batch; +use Google\Cloud\Core\SysvTrait; + /** * An external daemon script for executing the batch jobs. * @@ -32,8 +34,9 @@ */ class BatchDaemon { - use SysvTrait; + use BatchDaemonTrait; use HandleFailureTrait; + use SysvTrait; /* @var BatchRunner */ private $runner; @@ -51,6 +54,7 @@ class BatchDaemon * Prepare the descriptor spec and install signal handlers. * * @param string $entrypoint Daemon's entrypoint script. + * @throws \RuntimeException */ public function __construct($entrypoint) { diff --git a/src/Core/Batch/BatchDaemonTrait.php b/src/Core/Batch/BatchDaemonTrait.php new file mode 100644 index 000000000000..b24123116802 --- /dev/null +++ b/src/Core/Batch/BatchDaemonTrait.php @@ -0,0 +1,47 @@ +filePath = sprintf( + self::FILE_PATH_TEMPLATE, + sys_get_temp_dir(), + $fileName + ); + } + + /** + * Acquires a lock that will block until released. + * + * @return bool + * @throws \RuntimeException If the lock fails to be acquired. + */ + public function acquire() + { + if ($this->handle) { + return true; + } + + $this->handle = $this->initializeHandle(); + + if (!flock($this->handle, LOCK_EX)) { + fclose($this->handle); + $this->handle = null; + + throw new \RuntimeException('Failed to acquire lock.'); + } + + return true; + } + + /** + * Releases the lock. + * + * @throws \RuntimeException If the lock fails to release. + */ + public function release() + { + if ($this->handle) { + $released = flock($this->handle, LOCK_UN); + fclose($this->handle); + $this->handle = null; + + if (!$released) { + throw new \RuntimeException('Failed to release lock.'); + } + } + } + + /** + * Initializes the handle. + * + * @return resource + * @throws \RuntimeException If the lock file fails to open. + */ + private function initializeHandle() + { + $handle = @fopen($this->filePath, 'c'); + + if (!$handle) { + throw new \RuntimeException('Failed to open lock file.'); + } + + return $handle; + } +} diff --git a/src/Core/Lock/LockInterface.php b/src/Core/Lock/LockInterface.php index 2c0c98bab090..b328de036a94 100644 --- a/src/Core/Lock/LockInterface.php +++ b/src/Core/Lock/LockInterface.php @@ -40,8 +40,8 @@ public function release(); /** * Execute a callable within a lock. * + * @param callable $func The callable to execute. * @return mixed - * @throws \RuntimeException */ public function synchronize(callable $func); } diff --git a/src/Core/Lock/LockTrait.php b/src/Core/Lock/LockTrait.php new file mode 100644 index 000000000000..e20a0fda9f31 --- /dev/null +++ b/src/Core/Lock/LockTrait.php @@ -0,0 +1,68 @@ +acquire()) { + try { + $result = $func(); + } catch (\Exception $ex) { + $exception = $ex; + } + $this->release(); + } + + if ($exception) { + throw $exception; + } + + return $result; + } +} diff --git a/src/Core/Lock/SemaphoreLock.php b/src/Core/Lock/SemaphoreLock.php new file mode 100644 index 000000000000..c960cc9b09a2 --- /dev/null +++ b/src/Core/Lock/SemaphoreLock.php @@ -0,0 +1,120 @@ +isSysvIPCLoaded()) { + throw new \RuntimeException('SystemV IPC extensions are required.'); + } + + if (!is_int($key)) { + throw new \InvalidArgumentException('The provided key must be an integer.'); + } + + $this->key = $key; + } + + /** + * Acquires a lock that will block until released. + * + * @return bool + * @throws \RuntimeException If the lock fails to be acquired. + */ + public function acquire() + { + if ($this->semaphoreId) { + return true; + } + + $this->semaphoreId = $this->initializeId(); + + if (!sem_acquire($this->semaphoreId)) { + $this->semaphoreId = null; + + throw new \RuntimeException('Failed to acquire lock.'); + } + + return true; + } + + /** + * Releases the lock. + * + * @throws \RuntimeException If the lock fails to release. + */ + public function release() + { + if ($this->semaphoreId) { + $released = sem_release($this->semaphoreId); + $this->semaphoreId = null; + + if (!$released) { + throw new \RuntimeException('Failed to release lock.'); + } + } + } + + /** + * Initializes the semaphore ID. + * + * @return resource + * @throws \RuntimeException If semaphore ID fails to generate. + */ + private function initializeId() + { + $semaphoreId = sem_get($this->key); + + if (!$semaphoreId) { + throw new \RuntimeException('Failed to generate semaphore ID.'); + } + + return $semaphoreId; + } +} diff --git a/src/Core/Lock/SymfonyLockAdapter.php b/src/Core/Lock/SymfonyLockAdapter.php index da6667c6c769..f48237a8d7bf 100644 --- a/src/Core/Lock/SymfonyLockAdapter.php +++ b/src/Core/Lock/SymfonyLockAdapter.php @@ -24,6 +24,8 @@ */ class SymfonyLockAdapter implements LockInterface { + use LockTrait; + /** * @var SymfonyLockInterface */ @@ -65,31 +67,4 @@ public function release() throw new \RunTimeException($ex->getMessage()); } } - - /** - * Execute a callable within a lock. - * - * @return mixed - * @throws \RuntimeException - */ - public function synchronize(callable $func) - { - $result = null; - $exception = null; - - if ($this->acquire()) { - try { - $result = $func(); - } catch (\Exception $ex) { - $exception = $ex; - } - $this->release(); - } - - if ($exception) { - throw $exception; - } - - return $result; - } } diff --git a/src/Core/Batch/SysvTrait.php b/src/Core/SysvTrait.php similarity index 77% rename from src/Core/Batch/SysvTrait.php rename to src/Core/SysvTrait.php index c3e92f018c10..10a67e13c5c1 100644 --- a/src/Core/Batch/SysvTrait.php +++ b/src/Core/SysvTrait.php @@ -15,7 +15,7 @@ * limitations under the License. */ -namespace Google\Cloud\Core\Batch; +namespace Google\Cloud\Core; /** * A utility trait related to System V IPC. @@ -27,8 +27,6 @@ */ trait SysvTrait { - private static $typeDirect = 1; - private static $typeFile = 2; private static $productionKey = 'P'; /** @@ -36,8 +34,7 @@ trait SysvTrait * * Set GOOGLE_CLOUD_SYSV_ID envvar to change the base id. * - * @param int $idNum An id number of the job - * + * @param int $idNum An id number. * @return int */ private function getSysvKey($idNum) @@ -61,19 +58,4 @@ private function isSysvIPCLoaded() && extension_loaded('sysvsem') && extension_loaded('sysvshm'); } - - /** - * Returns if the BatchDaemon is supposed running. - * - * @return bool - */ - private function isDaemonRunning() - { - $isDaemonRunning = filter_var( - getenv('IS_BATCH_DAEMON_RUNNING'), - FILTER_VALIDATE_BOOLEAN - ); - - return $isDaemonRunning !== false; - } } diff --git a/src/Spanner/Session/CacheSessionPool.php b/src/Spanner/Session/CacheSessionPool.php index 90c1923e3f15..e49a117872de 100644 --- a/src/Spanner/Session/CacheSessionPool.php +++ b/src/Spanner/Session/CacheSessionPool.php @@ -18,12 +18,12 @@ namespace Google\Cloud\Spanner\Session; use Google\Cloud\Core\Exception\NotFoundException; +use Google\Cloud\Core\Lock\FlockLock; use Google\Cloud\Core\Lock\LockInterface; -use Google\Cloud\Core\Lock\SymfonyLockAdapter; +use Google\Cloud\Core\Lock\SemaphoreLock; +use Google\Cloud\Core\SysvTrait; use Google\Cloud\Spanner\Database; use Psr\Cache\CacheItemPoolInterface; -use Symfony\Component\Lock\Factory; -use Symfony\Component\Lock\Store\FlockStore; /** * This session pool implementation accepts a PSR-6 compatible cache @@ -63,16 +63,6 @@ * implementations please see the * [Packagist PHP Package Repository](https://packagist.org/providers/psr/cache-implementation). * - * Furthermore, [Symfony's Lock Component](https://github.com/symfony/lock) is - * also required to be installed as a separate dependency. In our current alpha - * state with Spanner we are relying on the following dev commit: - * - * `composer require symfony/lock:3.3.x-dev#1ba6ac9` - * - * As development continues, this dependency on a dev-master branch will be - * discontinued. Please also note, since this is a dev-master dependency it may - * require modifications to your composer minimum-stability settings. - * * Example: * ``` * use Google\Cloud\Spanner\SpannerClient; @@ -90,6 +80,8 @@ */ class CacheSessionPool implements SessionPoolInterface { + use SysvTrait; + const CACHE_KEY_TEMPLATE = 'cache-session-pool.%s.%s.%s'; const DURATION_TWENTY_MINUTES = 1200; @@ -112,7 +104,7 @@ class CacheSessionPool implements SessionPoolInterface private $cacheItemPool; /** - * @var string + * @var string|null */ private $cacheKey; @@ -122,7 +114,7 @@ class CacheSessionPool implements SessionPoolInterface private $config; /** - * @var Database + * @var Database|null */ private $database; @@ -145,7 +137,9 @@ class CacheSessionPool implements SessionPoolInterface * **Defaults to** `0.5`. Ignored when $shouldWaitForSession is * `false`. * @type LockInterface $lock A lock implementation capable of blocking. - * **Defaults to** an flock based implementation. + * **Defaults to** a semaphore based implementation if the + * required extensions are installed, otherwise an flock based + * implementation. * } * @throws \InvalidArgumentException */ @@ -153,11 +147,6 @@ public function __construct(CacheItemPoolInterface $cacheItemPool, array $config { $this->cacheItemPool = $cacheItemPool; $this->config = $config + self::$defaultConfig; - - if (!isset($this->config['lock'])) { - $this->config['lock'] = $this->getDefaultLock(); - } - $this->validateConfig(); } @@ -507,6 +496,10 @@ public function setDatabase(Database $database) $identity['instance'], $identity['database'] ); + + if (!isset($this->config['lock'])) { + $this->config['lock'] = $this->getDefaultLock(); + } } /** @@ -746,26 +739,16 @@ private function waitForNextAvailableSession() * Get the default lock. * * @return LockInterface - * @throws \RunTimeException */ private function getDefaultLock() { - if (!class_exists(FlockStore::class)) { - throw new \RuntimeException( - 'The symfony/lock component must be installed in order for ' . - 'a default lock to be assumed. Please run the following from ' . - 'the command line: composer require symfony/lock:3.3.x-dev#1ba6ac9. ' . - 'Please note, since this is a dev-master dependency it may ' . - 'require modifications to your composer minimum-stability ' . - 'settings.' + if ($this->isSysvIPCLoaded()) { + return new SemaphoreLock( + $this->getSysvKey(crc32($this->cacheKey)) ); } - $store = new FlockStore(sys_get_temp_dir()); - - return new SymfonyLockAdapter( - (new Factory($store))->createLock($this->cacheKey) - ); + return new FlockLock($this->cacheKey); } /** @@ -788,7 +771,7 @@ private function validateConfig() throw new \InvalidArgumentException('minSessions cannot exceed maxSessions'); } - if (!$this->config['lock'] instanceof LockInterface) { + if (isset($this->config['lock']) && !$this->config['lock'] instanceof LockInterface) { throw new \InvalidArgumentException( 'The lock must implement Google\Cloud\Core\Lock\LockInterface' ); diff --git a/src/Spanner/composer.json b/src/Spanner/composer.json index 258c4a020131..ce0c2ec237ed 100644 --- a/src/Spanner/composer.json +++ b/src/Spanner/composer.json @@ -9,9 +9,6 @@ "google/gax": "^0.21.0", "google/proto-client": "^0.21.0" }, - "suggest": { - "symfony/lock": "Required for the default session handler. Should be included as follows: 3.3.x-dev#1ba6ac9" - }, "extra": { "component": { "id": "cloud-spanner", diff --git a/tests/unit/Core/Batch/BatchDaemonTraitTest.php b/tests/unit/Core/Batch/BatchDaemonTraitTest.php new file mode 100644 index 000000000000..32694bca5d67 --- /dev/null +++ b/tests/unit/Core/Batch/BatchDaemonTraitTest.php @@ -0,0 +1,62 @@ +impl = new MyBatchDaemonTraitClass(); + } + + public function testIsDaemonRunning() + { + // Clear the env + $orig = getenv('IS_BATCH_DAEMON_RUNNING'); + try { + putenv('IS_BATCH_DAEMON_RUNNING'); + $this->assertFalse($this->impl->isDaemonRunning()); + putenv('IS_BATCH_DAEMON_RUNNING=true'); + $this->assertTrue($this->impl->isDaemonRunning()); + } finally { + if ($orig === false) { + putenv('IS_BATCH_DAEMON_RUNNING'); + } else { + putenv('IS_BATCH_DAEMON_RUNNING=' . $orig); + } + } + } +} + +class MyBatchDaemonTraitClass +{ + use BatchDaemonTrait { + isDaemonRunning as privateIsDaemonRunning; + } + + function isDaemonRunning() + { + return $this->privateIsDaemonRunning(); + } +} diff --git a/tests/unit/Core/Batch/SysvConfigStorageTest.php b/tests/unit/Core/Batch/SysvConfigStorageTest.php index 23b95da2ed99..87914bc45f85 100644 --- a/tests/unit/Core/Batch/SysvConfigStorageTest.php +++ b/tests/unit/Core/Batch/SysvConfigStorageTest.php @@ -18,8 +18,8 @@ namespace Google\Cloud\Tests\Unit\Core\Batch; use Google\Cloud\Core\Batch\BatchConfig; -use Google\Cloud\Core\Batch\SysvTrait; use Google\Cloud\Core\Batch\SysvConfigStorage; +use Google\Cloud\Core\SysvTrait; /** * @group core diff --git a/tests/unit/Core/Batch/SysvProcessorTest.php b/tests/unit/Core/Batch/SysvProcessorTest.php index 364ba4692241..1661f68275e6 100644 --- a/tests/unit/Core/Batch/SysvProcessorTest.php +++ b/tests/unit/Core/Batch/SysvProcessorTest.php @@ -17,8 +17,9 @@ namespace Google\Cloud\Tests\Unit\Core\Batch; +use Google\Cloud\Core\Batch\BatchDaemonTrait; use Google\Cloud\Core\Batch\SysvProcessor; -use Google\Cloud\Core\Batch\SysvTrait; +use Google\Cloud\Core\SysvTrait; /** * @group core @@ -26,6 +27,7 @@ */ class SysvProcessorTest extends \PHPUnit_Framework_TestCase { + use BatchDaemonTrait; use SysvTrait; private $submitter; diff --git a/tests/unit/Core/Lock/CommonLockTrait.php b/tests/unit/Core/Lock/CommonLockTrait.php new file mode 100644 index 000000000000..c03fd4eee5e7 --- /dev/null +++ b/tests/unit/Core/Lock/CommonLockTrait.php @@ -0,0 +1,72 @@ +lock = $lock; + } + + public function testAcquireAndReleaseLock() + { + $this->assertTrue($this->lock->acquire()); + $this->lock->release(); + } + + public function testAcquireSameLockBeforeRelease() + { + $this->assertTrue($this->lock->acquire()); + $this->assertTrue($this->lock->acquire()); + + $this->lock->release(); + } + + public function testSynchronizeLock() + { + $return = $this->lock->synchronize(function () { + return true; + }); + + $this->assertTrue($return); + } + + /** + * @expectedException \Exception + */ + public function testSynchronizeLockThrowsException() + { + $this->lock->synchronize(function () { + throw new \Exception(); + }); + } +} + diff --git a/tests/unit/Core/Lock/FlockLockTest.php b/tests/unit/Core/Lock/FlockLockTest.php new file mode 100644 index 000000000000..5f108223e68d --- /dev/null +++ b/tests/unit/Core/Lock/FlockLockTest.php @@ -0,0 +1,80 @@ +setLock(new FlockLock(self::LOCK_NAME)); + } + + /** + * @expectedException \InvalidArgumentException + */ + public function testThrowsExceptionWithInvalidFileName() + { + new FlockLock(123); + } + + /** + * @expectedException \RuntimeException + * @expectedExceptionMessage Failed to acquire lock. + */ + public function testThrowsExceptionWhenFlockFailsOnAcquire() + { + MockValues::$flockReturnValue = false; + $this->lock->acquire(); + } + + /** + * @expectedException \RuntimeException + * @expectedExceptionMessage Failed to release lock. + */ + public function testThrowsExceptionWhenFlockFailsOnRelease() + { + $this->lock->acquire(); + MockValues::$flockReturnValue = false; + $this->lock->release(); + } + + /** + * @expectedException \RuntimeException + * @expectedExceptionMessage Failed to open lock file. + */ + public function testThrowsExceptionWhenFopenFails() + { + MockValues::$fopenReturnValue = false; + $this->lock->acquire(); + } +} + diff --git a/tests/unit/Core/Lock/MockGlobals.php b/tests/unit/Core/Lock/MockGlobals.php new file mode 100644 index 000000000000..2f4982224f26 --- /dev/null +++ b/tests/unit/Core/Lock/MockGlobals.php @@ -0,0 +1,62 @@ +isSysvIPCLoaded()) { + $this->markTestSkipped( + 'Skipping because SystemV IPC extensions are not loaded' + ); + } + + $this->setLock(new SemaphoreLock(1)); + MockValues::initialize(); + } + + /** + * @expectedException \InvalidArgumentException + */ + public function testThrowsExceptionWithInvalidKey() + { + new SemaphoreLock('abc'); + } + + /** + * @expectedException \RuntimeException + * @expectedExceptionMessage Failed to acquire lock. + */ + public function testThrowsExceptionWhenSemAcquireFailsOnAcquire() + { + MockValues::$sem_acquireReturnValue = false; + $this->lock->acquire(); + } + + /** + * @expectedException \RuntimeException + * @expectedExceptionMessage Failed to release lock. + */ + public function testThrowsExceptionWhenSemReleaseFailsOnRelease() + { + $this->lock->acquire(); + MockValues::$sem_releaseReturnValue = false; + $this->lock->release(); + } + + /** + * @expectedException \RuntimeException + * @expectedExceptionMessage Failed to generate semaphore ID. + */ + public function testThrowsExceptionWhenSemGetFails() + { + MockValues::$sem_getReturnValue = false; + $this->lock->acquire(); + } +} + diff --git a/tests/unit/Core/Batch/SysvTraitTest.php b/tests/unit/Core/SysvTraitTest.php similarity index 65% rename from tests/unit/Core/Batch/SysvTraitTest.php rename to tests/unit/Core/SysvTraitTest.php index bd0b3509c9ea..e05aff4839ae 100644 --- a/tests/unit/Core/Batch/SysvTraitTest.php +++ b/tests/unit/Core/SysvTraitTest.php @@ -15,13 +15,12 @@ * limitations under the License. */ -namespace Google\Cloud\Tests\Unit\Core\Batch; +namespace Google\Cloud\Tests\Unit\Core; -use Google\Cloud\Core\Batch\SysvTrait; +use Google\Cloud\Core\SysvTrait; /** * @group core - * @group batch */ class SysvTraitTest extends \PHPUnit_Framework_TestCase { @@ -32,9 +31,10 @@ public function setUp() public function testGetSysvKey() { - if (! $this->impl->isSysvIPCLoaded()) { + if (!$this->impl->isSysvIPCLoaded()) { $this->markTestSkipped( - 'SysV IPC extensions are not available, skipped'); + 'SysV IPC extensions are not available, skipped' + ); } $key1 = $this->impl->getSysvKey(1); $key2 = $this->impl->getSysvKey(2); @@ -48,39 +48,15 @@ public function testIsSysvIPCLoaded() && extension_loaded('sysvshm'); $this->assertEquals($expected, $this->impl->isSysvIPCLoaded()); } - - public function testIsDaemonRunning() - { - // Clear the env - $orig = getenv('IS_BATCH_DAEMON_RUNNING'); - try { - putenv('IS_BATCH_DAEMON_RUNNING'); - $this->assertFalse($this->impl->isDaemonRunning()); - putenv('IS_BATCH_DAEMON_RUNNING=true'); - $this->assertTrue($this->impl->isDaemonRunning()); - } finally { - if ($orig === false) { - putenv('IS_BATCH_DAEMON_RUNNING'); - } else { - putenv('IS_BATCH_DAEMON_RUNNING=' . $orig); - } - } - } } class MySysvClass { use SysvTrait { - isDaemonRunning as privateIsDaemonRunning; isSysvIPCLoaded as privateIsSysvIPCLoaded; getSysvKey as privateGetSysvKey; } - function isDaemonRunning() - { - return $this->privateIsDaemonRunning(); - } - function isSysvIPCLoaded() { return $this->privateIsSysvIPCLoaded(); diff --git a/tests/unit/Spanner/Session/CacheSessionPoolTest.php b/tests/unit/Spanner/Session/CacheSessionPoolTest.php index c0f402488a62..452281eec4cb 100644 --- a/tests/unit/Spanner/Session/CacheSessionPoolTest.php +++ b/tests/unit/Spanner/Session/CacheSessionPoolTest.php @@ -44,6 +44,7 @@ class CacheSessionPoolTest extends \PHPUnit_Framework_TestCase public function setUp() { $this->checkAndSkipGrpcTests(); + putenv('GOOGLE_CLOUD_SYSV_ID=U'); $this->time = time(); }