Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[Lock] Add MysqlStore #45982

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 11 commits into
base: 7.4
Choose a base branch
Loading
from
Open

[Lock] Add MysqlStore #45982

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
6a18831
Add MysqlStore
rtek Apr 9, 2022
f849ecb
Fix changelog
rtek Apr 9, 2022
2f26e01
Fix fabbot
rtek Apr 10, 2022
6954fc6
Add dsn support
rtek Apr 11, 2022
4730f91
Move state into store from key
rtek Apr 11, 2022
d28db69
Use the connection id for lock state
rtek Apr 11, 2022
2def8dd
Fix lazy property semantics
rtek Apr 11, 2022
cda417a
Move lock state into key from store
rtek Apr 11, 2022
827545a
Handle mysql restarts
rtek Apr 11, 2022
9a32d39
Allow tests to use a dsn instead of host
rtek Apr 11, 2022
e2f0faa
Address feedback
rtek Apr 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Move state into store from key
  • Loading branch information
@rtek
rtek committed Apr 11, 2022
commit 4730f912f54a0f46b04d54f542d646344188759d
46 changes: 24 additions & 22 deletions 46 src/Symfony/Component/Lock/Store/MysqlStore.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ class MysqlStore implements PersistingStoreInterface

private array $options;

/** @var bool[] */
private static array $locksAcquired = [];

public function __construct(\PDO|string $connOrDsn, array $options = [])
{
if ($connOrDsn instanceof \PDO) {
Expand All @@ -46,22 +49,20 @@ public function __construct(\PDO|string $connOrDsn, array $options = [])

public function save(Key $key): void
{
if ($key->hasState(__CLASS__)) {
$id = $this->getLockId($key);

if (self::$locksAcquired[$id] ?? false) {
return;
}

// mysql limits lock name length to 64 chars
$name = (string) $key;
$name = \strlen($name) > 64 ? hash('xxh128', $name) : $name;

$stmt = $this->conn->prepare('SELECT IF(IS_USED_LOCK(:name) = CONNECTION_ID(), -1, GET_LOCK(:name, 0))');
Copy link
Member

@GromNaN GromNaN Apr 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PostgreSqlStore uses the InMemoryStore to avoid duplicate lock on the same connection.

symfony/src/Symfony/Component/Lock/Store/PostgreSqlStore.php

Line 288 in 1f514f9

return self::$storeRegistry[$namespace] ?? self::$storeRegistry[$namespace] = new InMemoryStore();

Copy link
Contributor Author

@rtek rtek Apr 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on this requirement?

Im concerned that using the InMemoryStore can cause unexpected behavior in situations where either:

  1. A combined store is used with with Mysql and Postgres
  2. A the same key is passed to two LockFactory::createLockFromKey, one using Mysql and the other using Postgres

I moved the state into the store since the tests assert that exists() is bound to the Key instance, while save() is bound to the Key::$resource. Does this work?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. They should not share the same instance of InMemoryStore

$stmt->bindValue(':name', $name, \PDO::PARAM_STR);
$stmt->bindValue(':name', self::getLockName($key), \PDO::PARAM_STR);
$stmt->execute();
$result = $stmt->fetchColumn();

// lock acquired
if (1 === $result) {
$key->setState(__CLASS__, $name);
self::$locksAcquired[$id] = true;

return;
}
Expand All @@ -84,28 +85,16 @@ public function putOffExpiration(Key $key, float $ttl): void

public function delete(Key $key): void
{
if (!$key->hasState(__CLASS__)) {
return;
}

$stmt = $this->conn->prepare('DO RELEASE_LOCK(:name)');
$stmt->bindValue(':name', $key->getState(__CLASS__), \PDO::PARAM_STR);
$stmt->bindValue(':name', self::getLockName($key), \PDO::PARAM_STR);
$stmt->execute();

$key->removeState(__CLASS__);
unset(self::$locksAcquired[$this->getLockId($key)]);
}

public function exists(Key $key): bool
{
if (!$key->hasState(__CLASS__)) {
return false;
}

$stmt = $this->conn->prepare('SELECT IF(IS_USED_LOCK(:name) = CONNECTION_ID(), 1, 0)');
$stmt->bindValue(':name', $key->getState(__CLASS__), \PDO::PARAM_STR);
$stmt->execute();

return 1 === $stmt->fetchColumn();
return self::$locksAcquired[$this->getLockId($key)] ?? false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's wrong, the implementation must assert that the lock is still present in the backend. (Ie the database restart, the network is down, ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wont be possible since GET_LOCK is bound to the connection so its always released when the connection is terminated.

Copy link
Member

@jderusse jderusse Apr 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well.. getLockId is based on connection_id() which is (for MySQL 8 at least) an auto-increment that is reset when server starts.

This means, that each time the server restarts, the application might retrieve the connection_id previously affected to another process. Which makes the key used in self::$locksAcquired untrustable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should do it.

I cant add this test case to the since it requires rebooting mysql but I did run it manually to confirm proper handling even if the connection id is the same

class ManualMysqlStore extends MysqlStoreTest
{
    public function testMysqlClosedConnection()
    {
        // manually restart mysql to reset conn id counter
        $pdo = $this->getPdo();
        $storeA = new MysqlStore($pdo);
        $key = new Key('foo');
        $storeA->save($key);

        $connIdA = $pdo->query('SELECT CONNECTION_ID()')->fetchColumn();

        while (true) {
            try {
                $this->assertTrue($storeA->exists($key));
                sleep(1);
                // manually restart mysql to reset conn id counter
            } catch (\PDOException $e) {
                // mysql has gone away
                break;
            }
        }

        $pdo = $this->getPdo();
        $storeB = new MysqlStore($pdo);
        $connIdB = $pdo->query('SELECT CONNECTION_ID()')->fetchColumn();

        $this->assertFalse($storeB->exists($key));
        $this->assertSame($connIdA, $connIdB);
    }
}

}

private function getConnection(): \PDO
Expand All @@ -130,4 +119,17 @@ private function assertMysqlDriver(): void
throw new InvalidArgumentException(sprintf('The adapter "%s" does not support the "%s" driver.', __CLASS__, $driver));
}
}

private function getLockId(Key $key): string
{
return spl_object_id($this->getConnection()).'_'.spl_object_id($key);
}

private static function getLockName(Key $key): string
{
// mysql limits lock name length to 64 chars
$name = (string) $key;

return \strlen($name) > 64 ? hash('xxh128', $name) : $name;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hashing only longer names will mean that there is 2 resources that share the same MySQL lock name

}
}
13 changes: 0 additions & 13 deletions 13 src/Symfony/Component/Lock/Tests/Store/MysqlStoreTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,19 +53,6 @@ public function testExceptionModeRequirement()
new MysqlStore($pdo);
}

public function testSelfConflictException()
{
$store = $this->getStore();
$store->save(new Key('foo'));

try {
$store->save(new Key('foo'));
$this->fail('Expected exception: '.LockConflictedException::class);
} catch (LockConflictedException $e) {
$this->assertStringContainsString('acquired by this', $e->getMessage());
}
}

public function testOtherConflictException()
{
$storeA = $this->getStore();
Expand Down
Morty Proxy This is a proxified and sanitized view of the page, visit original site.