[Lock] Added Symfony/Component/Lock/Store/MongoDbStore #27346

kralos · 2018-05-23T05:40:04Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes (requires `ext-mongodb` and `mongodb/mongodb` to test)
Fixed tickets	#27345
License	MIT
Doc PR	symfony/symfony-docs#9807

Testing caveat
In order to test this, the test environment needs ext-mongodb and mongodb/mongodb. Travis has ext-mongodb but Appveyor doesn't meaning I can't require-dev mongodb/mongodb. I had a look at Symfony\Component\HttpFoundation\Session\Storage\Handler\MongoDbSessionHandler since it poses the same issue and neither symfony/symfony/src/Symfony/Component/HttpFoundation/composer.json nor symfony/symfony/composer.json require-dev mongodb/mongodb. They both skip mongo based tests in the ci environments. I have therefore chosen to omit require-dev mongodb/mongodb.

I have both written the test and tested Symfony\Component\Lock\Store\MongoDbStore and it does pass in an environment with ext-mongodb and mongodb/mongodb.

Description
We should support Semaphore Locks with a MongoDB back end to allow those that already use MongoDB as a distributed storage engine.

Symfony already partially supports MongoDB for session storage: Symfony\Component\HttpFoundation\Session\Storage\Handler\MongoDbSessionHandler

Example

$client = new MongoDb\Client();

$store = new Symfony\Component\Lock\Store\MongoDbStore(
    $client
    array(
        'database' => 'my-app',
    )
);
$lockFactory = new Symfony\Component\Lock\Factory($store);
$lock = $lockFactory->createLock('my-resource');

kralos · 2018-05-23T06:04:38Z

@nicolas-grekas what's the deal with adding ext-mongodb to symfony/symfony's appveyor build environment? Could you help me out?

nicolas-grekas · 2018-05-23T06:53:20Z

No need for the ext on appveyor. Just skip tests there. An @requires extension mongodbannotation on the test class should do it.

travis env has ext-mongodb but appveyor doesn't since MongoDbSessionHandler does the same i've stripped the dev requirement for mongodb/mongodb

jderusse

I'm not familiar with MongoDb, I'm OK with the current implementation (with few comments),

BUT I've many concerns about the reliability of MongoDb. It's wellknown for not beeing ACID, whereas Atomicity is a requirements. What's about a MongoDb cluster configured with sharding and replication?

jderusse · 2018-05-31T08:46:03Z

src/Symfony/Component/Lock/Store/MongoDbStore.php

+     * collection: The name of the collection [default: lock]
+     * resource_field: The field name for storing the lock id [default: _id] MUST be uniquely indexed if you chage it
+     * token_field: The field name for storing the lock token [default: token]
+     * acquired_field: The field name for storing the acquisition timestamp [default: acquired_at]


what is the purpose of this field?

I've removed it. I found it useful to debug / see when a lock was acquired. Probably not that useful in operation.

jderusse · 2018-05-31T08:51:43Z

src/Symfony/Component/Lock/Store/MongoDbStore.php

+     * the locks in the database as described below:
+     *
+     * A TTL collections can be used on MongoDB 2.2+ to cleanup expired locks
+     * automatically. Such an index can for example look like this:


If the expiration date is computed by the application, then you should warn the developer that a clock drift between the app server and the database server may delete the lock to early

I've added a comment to the phpdoc and symfony-docs. I also recommended setting the expireAfterSeconds to a value greater than 0. The actual expiry is already handled by the application (not the DBMS's clocks). I have updated my example to 60 seconds.

IMHO this safety window should be a constructor parameter. Because the same issue exist on the save method when 2 clients are not time synchronized

Currently the recommendation is to manually create the TTL index. If I were to implement this as a constructor parameter in code we would be creating / updating the index on every execution. I'm not sure this is a good idea.

I would simply set the TTL to 0, and append this "clockDrift" parameter to the expiredAt (expiredAt = now + lockTTL + clockDrift). It would both solve the purge issue by Mongo and the race condition by 2 clients.

I've moved clock discrepancy handling to a constructor option called drift instead of relying on a TTL index delay.

jderusse · 2018-05-31T08:53:35Z

src/Symfony/Component/Lock/Store/MongoDbStore.php

+        $expiry = $this->createDateTime(microtime(true) + $this->initialTtl);
+
+        $filter = array(
+            $this->options['resource_field'] => (string) $key,


is there a restriction on key length or key format?

Yes, 1024 bytes including structure overhead (e.g. int / string) I've added this to symfony-docs under the Reliability section and to the phpdoc

jderusse · 2018-05-31T08:54:48Z

src/Symfony/Component/Lock/Store/MongoDbStore.php

+        $update = array(
+            '$set' => array(
+                $this->options['resource_field'] => (string) $key,
+                $this->options['token_field'] => $this->getToken($key),


the method $this->getToken($key) is already called at line 108

jderusse · 2018-05-31T08:56:26Z

src/Symfony/Component/Lock/Store/MongoDbStore.php

+
+        $key->reduceLifetime($this->initialTtl);
+        try {
+            $this->getCollection()->updateOne($filter, $update, $options);


Is it atomic? on every version of mongodb? What's about réplication propagation?

MongoDB is atomic on a singular statement / document only. Hence the use of a update with filter (1 statement). MongoDB ^4.0 introduces multi statement transactions however this implementation does not rely on that. It should be atomic on all versions. Replication propagation can be controlled with a statement level options called a write concern. I'm adding support for that now. By default, mongo will read and write only to the PRIMARY node in a replica set (this is equivalent of reading and writing only to the master of a replicated RDBMS with replication slaves).

With regards to writeConcern, readConcern and readPreference I've opted to leave this to the collection fallback placing the onus on the DBA to setup the collection with alternate settings if desired. The default when left unspecified is to read / write ONLY to the PRIMARY of a replica set. This is safe for the use of locks (it's like only reading / writing to a master in an RDBMS) however an alternate strategy could be left to the DBA depending on the number of nodes they have.

I have added some documentation regarding this decision to symfony-docs

jderusse · 2018-05-31T08:59:45Z

src/Symfony/Component/Lock/Store/MongoDbStore.php

+     * db.lock.update(
+     *     {
+     *         _id: "test",
+     *         expires_at: {


whats i the purpose of this doc block (which is not up to date with the current implementation BTW)

It was more to help with seeing how the code translated to a MongoDB statement when I was supporting multiple drivers. I have since stripped the multiple driver support (Fabian said don't support PHP ^5 / symfony ^3 so I dropped it). I've now removed these phpdocs

jderusse · 2018-05-31T09:03:33Z

src/Symfony/Component/Lock/Store/MongoDbStore.php

+            $this->options['resource_field'] => (string) $key,
+            $this->options['token_field'] => $this->getToken($key),
+            $this->options['expiry_field'] => array(
+                '$gte' => $this->createDateTime(),


I'd rather define a now variable at the beggining of the method, then use this variable as a reference for every computes

Makes sense, I've fixed this

jderusse · 2018-05-31T09:04:17Z

src/Symfony/Component/Lock/Store/MongoDbStore.php

+        $key->reduceLifetime($ttl);
+        try {
+            $this->getCollection()->updateOne($filter, $update, $options);
+        } catch (\MongoDB\Driver\Exception\BulkWriteException $e) {


improved exception handling as mentioned above

jderusse · 2018-05-31T09:05:15Z

src/Symfony/Component/Lock/Store/MongoDbStore.php

+        try {
+            $result = $this->getCollection()->deleteOne($filter);
+        } catch (\MongoDB\Driver\Exception\BulkWriteException $e) {
+            throw new LockConflictedException('Failed to delete lock', 0, $e);


deletion does not throw ConflictException

jderusse · 2018-05-31T09:07:40Z

src/Symfony/Component/Lock/Store/MongoDbStore.php

+     * garbage-collection. Alternatively it's possible to automatically expire
+     * the locks in the database as described below:
+     *
+     * A TTL collections can be used on MongoDB 2.2+ to cleanup expired locks


The current implementation does not purge expired locks. I'd rather use the verb MUST BE used,

…figurable fields

…n 'drift'

kralos · 2018-05-31T13:09:57Z

@jderusse should be good for a second look. Thanks for you feedback so far, really helpful.

jderusse · 2018-05-31T18:15:29Z

src/Symfony/Component/Lock/Store/MongoDbStore.php

+     * must not exceed 1024 bytes including structural overhead.
+     * @see https://docs.mongodb.com/manual/reference/limits/#Index-Key-Limit
+     *
+     * @param float $initialTtl The expiration delay of locks in seconds


Why $initialTtl is a constructor argument, whereas every other options are passed into $options?

I'll move it to $options.

I've been thinking it would be simpler to remove the drift option in favour of asking the developer set a slightly higher lockTTL. This would both simplify the implementation and probably make more sense to the developer rather than trying to explain why drift is a separate option. I would have to document the danger of clock drift against this Store. What are your thoughts?

…TTL higher

jderusse · 2018-06-04T06:56:06Z

src/Symfony/Component/Lock/Store/MongoDbStore.php

+     *
+     * A TTL index MUST BE used on MongoDB 2.2+ to automatically clean up expired locks.
+     *
+     *     db.lock.ensureIndex(


I suggest to provide a public function createCollection like in https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Cache/Traits/PdoTrait.php#L82 or https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php#L209 or https://github.com/symfony/symfony/pull/27456/files

I've added a method to createTTLIndex(). There's no need in MongoDB to create the collection, they're created on the fly by the database.

jderusse · 2018-06-06T04:45:51Z

src/Symfony/Component/Lock/Store/MongoDbStore.php

+     *
+     * @see http://docs.mongodb.org/manual/tutorial/expire-data/
+     */
+    public function createTTLIndex(): bool


return false means a failure or "nothing updated"?
If it's a failure, I suggest to throw an exception instead.
What's about documenting the return?

My bad, I actually dug into this further and my return type bool was incorrect. I was reading the legacy driver documentation. The createIndex call actually does throw an exception if it fails. I have quickly tested this to verify and updated the phpdoc accordingly.

https://docs.mongodb.com/php-library/v1.3/reference/method/MongoDBCollection-createIndex/

jderusse · 2018-06-06T05:07:47Z

src/Symfony/Component/Lock/Store/MongoDbStore.php

+
+        $this->options = array_merge(array(
+            'collection' => 'lock',
+            'initialTtl' => 300.0,


I really wonder if options for lock should be merge with options for database connection?

See PdoTrait from cache components.

What is the opinion of other contributors?

I originally had initialTtl as a separate constructor parameter, I brought it into $options when you suggested drift should be.

I personally think it belongs outside of $options.

See also: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Cache/Adapter/PdoAdapter.php

jderusse · 2018-06-06T05:08:30Z

src/Symfony/Component/Lock/Store/MongoDbStore.php

+    private $collection;
+
+    /**
+     * @param \MongoDB\Client $mongo


Is there a way to pass a dns for lazy connection? See PdoTrait from cache components?

A MongoDB\Client instance is already lazy. I can turn off my database and still:

$client = new \MongoDB\Client(); $store = new \Symfony\Component\Lock\Store\MongoDbStore($client, [ 'database' => 'test', ]);

only if I add something like:

$store->createTTLIndex();

does it throw an exception.

…Index

nicolas-grekas · 2018-06-19T13:18:56Z

@kralos would you mind squashing all commits in one please?

nicolas-grekas · 2018-06-20T04:26:54Z

Why did you close?

nicolas-grekas · 2018-06-20T04:29:36Z

Oh, continued in #27648 thanks :)

kralos · 2018-06-25T03:53:49Z

Sorry, I broke this Pull request when squashing... has to open a new one.

This PR was merged into the 4.3-dev branch. Discussion ---------- [Lock] Added MongoDBStore | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes (requires `ext-mongodb` and `mongodb/mongodb` to test) | Fixed tickets | #27345 | License | MIT | Doc PR | symfony/symfony-docs#9807 **Testing caveat** In order to test this, the test environment needs `ext-mongodb` and `mongodb/mongodb`. I have both written the test and tested `Symfony\Component\Lock\Store\MongoDbStore` and it does pass in an environment with `ext-mongodb` and `mongodb/mongodb`. **Description** We should support Semaphore Locks with a MongoDB back end to allow those that already use MongoDB as a distributed storage engine. Symfony already partially supports MongoDB for session storage: `Symfony\Component\HttpFoundation\Session\Storage\Handler\MongoDbSessionHandler` **Example** ```php $client = new MongoDb\Client(); $store = new Symfony\Component\Lock\Store\MongoDbStore( $client array( 'database' => 'my-app', ) ); $lockFactory = new Symfony\Component\Lock\Factory($store); $lock = $lockFactory->createLock('my-resource'); ``` This is a squashed pull request of #27346 Commits ------- 9c04639 #27345 Added Lock/Store/MongoDbStore

symfony#27345 Added Symfony/Component/Lock/Store/MongoDbStore

157fcd7

carsonbot added Status: Needs Review Feature labels May 23, 2018

Joe Bennett added 2 commits May 23, 2018 15:43

symfony#27345 Fixed typo

e73f663

symfony#27345 fixed coding standards

3838412

kralos mentioned this pull request May 23, 2018

#27345 Added MongoDbStore Lock Store documentation symfony/symfony-docs#9807

Merged

symfony#27345 Removed dev requirement for mongodb/mongodb

1a660e6

travis env has ext-mongodb but appveyor doesn't since MongoDbSessionHandler does the same i've stripped the dev requirement for mongodb/mongodb

nicolas-grekas added this to the next milestone May 23, 2018

symfony#27345 Removed unnessasary reduceLifetime

d65fddf

kralos changed the title ~~#27345 Added Symfony/Component/Lock/Store/MongoDbStore~~ [Lock] Added Symfony/Component/Lock/Store/MongoDbStore May 25, 2018

jderusse reviewed May 31, 2018

View reviewed changes

jderusse mentioned this pull request May 31, 2018

[Lock] Add MysqlStore that use GET_LOCK #25578

Closed

Joe Bennett added 5 commits May 31, 2018 21:10

symfony#27345 Improved documentation, exception handling, removed con…

f3cc220

…figurable fields

symfony#27345 fixed typo and @param indentation

ec8d217

symfony#27345 fixed @param indentation

a1a3be7

symfony#27345 moved clock discrepancy handling to a constructor optio…

1b150ba

…n 'drift'

symfony#27345 fixed phpdoc spacing

73bb802

jderusse reviewed May 31, 2018

View reviewed changes

symfony#27345 removed drift option in favour or recommending setting …

5e86904

…TTL higher

jderusse reviewed Jun 4, 2018

View reviewed changes

Joe Bennett added 7 commits June 6, 2018 08:32

symfony#27345 Added public createTTLIndex method

8572c21

symfony#27345 Fixed coding standards

dbdad36

symfony#27345 Fixed coding standards

84c4d15

symfony#27345 Fixed coding standards

d577191

symfony#27345 Fixed coding standards

2ce68f5

symfony#27345 Improved documentation

5446b11

symfony#27345 Fixed spacing for fabbot

a9b85d6

Joe Bennett added 3 commits June 6, 2018 09:43

symfony#27345 reordered documentation in order of importance

6fdc602

symfony#27345 Fixed spacing for fabbot

9d80a0d

symfony#27345 Fixed spacing for fabbot

1ac702a

jderusse reviewed Jun 6, 2018

View reviewed changes

Joe Bennett added 8 commits June 6, 2018 15:15

symfony#27345 Updated return type and exception handling on createTTL…

4ba5b41

…Index

symfony#27345 Updated exception handling on createTTLIndex

58ef873

symfony#27345 Fixed spacing for fabbot

36c3af4

symfony#27345 Removed initialTtl from array and fixed unit test

168f194

symfony#27345 phpdoc adjusted for fabbot

abc72f8

symfony#27345 phpdoc adjusted for fabbot

e062bf2

symfony#27345 trigger ci retry

f2e23b4

symfony#27345 trigger ci retry

97e5711

kralos closed this Jun 19, 2018

kralos deleted the 27345-lock-mongodb branch June 19, 2018 22:34

kralos mentioned this pull request Jun 19, 2018

[Lock] Added MongoDBStore #27648

Merged

nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018

[Lock] Added Symfony/Component/Lock/Store/MongoDbStore #27346

[Lock] Added Symfony/Component/Lock/Store/MongoDbStore #27346

Conversation

kralos commented May 23, 2018 • edited Loading

kralos commented May 23, 2018

nicolas-grekas commented May 23, 2018

jderusse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kralos commented May 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kralos Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Jun 19, 2018

nicolas-grekas commented Jun 20, 2018

nicolas-grekas commented Jun 20, 2018

kralos commented Jun 25, 2018

kralos commented May 23, 2018 •

edited

Loading

kralos Jun 6, 2018 •

edited

Loading