Skip to content
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.

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

[Lock] Added MongoDBStore #27648

Merged
merged 1 commit into from Mar 29, 2019

Conversation

Projects
None yet
6 participants
@kralos
Copy link

commented Jun 19, 2018

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

$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

@nicolas-grekas nicolas-grekas changed the title #27345 [Lock] Added MongoDBStore [Lock] Added MongoDBStore Jun 20, 2018

@nicolas-grekas nicolas-grekas added this to the next milestone Jun 20, 2018

@kralos kralos closed this Jun 25, 2018

@kralos kralos force-pushed the kralos:27345-lock-mongodb branch from 6c017c4 to c871857 Jun 25, 2018

@kralos

This comment has been minimized.

Copy link
Author

commented Jun 25, 2018

Removed composer require-dev mongodb/mongodb from Component\Lock in case someone requires it and doesn't want to use MongoDbStore.
Renamed MongoDbClientTest to MongoDbStoreTest
Squashed commits

@kralos kralos reopened this Jun 25, 2018

@jderusse

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2018

require-dev are only installed when you run composer install on the component (every project that define the "lock component" as a dependency won't get packages defined in this section). This is the right place to add dependencies for developement or testing of the component.

By removing the dependency all the tests are flagged skipped

@kralos kralos force-pushed the kralos:27345-lock-mongodb branch from f290cc6 to 987548d Jun 25, 2018

@kralos

This comment has been minimized.

Copy link
Author

commented Jun 25, 2018

Ah ok, i've reinstated require-dev mongodb/mongodb in Symfony\Component\Lock

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jul 25, 2018

Personally, when this is contributed like here, I'm OK with adding this to the component directly, provided the implementation is top quality (double care for reviewers + author).

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

Could you please rebase? We prefer not merging PRs when they contain merge commits.

@kralos kralos force-pushed the kralos:27345-lock-mongodb branch from af48098 to 8692ca3 Sep 10, 2018

Show resolved Hide resolved src/Symfony/Component/Lock/Store/MongoDbStore.php Outdated
Show resolved Hide resolved src/Symfony/Component/Lock/Store/MongoDbStore.php Outdated
Show resolved Hide resolved src/Symfony/Component/Lock/Store/MongoDbStore.php Outdated
Show resolved Hide resolved src/Symfony/Component/Lock/StoreInterface.php Outdated
Show resolved Hide resolved src/Symfony/Component/Lock/Tests/Store/MongoDbStoreTest.php Outdated
Show resolved Hide resolved src/Symfony/Component/Lock/Tests/Store/MongoDbStoreTest.php Outdated
Show resolved Hide resolved src/Symfony/Component/Lock/Tests/Store/MongoDbStoreTest.php Outdated
Show resolved Hide resolved src/Symfony/Component/Lock/Tests/Store/MongoDbStoreTest.php Outdated
Show resolved Hide resolved phpunit.xml.dist
Show resolved Hide resolved src/Symfony/Component/Lock/Store/MongoDbStore.php Outdated

@kralos kralos force-pushed the kralos:27345-lock-mongodb branch 3 times, most recently from 304839f to 0b4abbb Sep 19, 2018

Show resolved Hide resolved phpunit.xml.dist
try {
$this->getCollection()->updateOne($filter, $update, $options);
} catch (WriteException $e) {
throw new LockConflictedException('Failed to acquire lock', 0, $e);

This comment has been minimized.

Copy link
@GaneshChandrasekaran

GaneshChandrasekaran Oct 4, 2018

I think you forgot to handle the lock conflicted case like in the the putoffExpiration function exception handling. This is just wrapping all exceptions as LockConflictedException.

This comment has been minimized.

Copy link
@kralos

kralos Oct 5, 2018

Author

yeah thinking about this some more, you are correct. I have added handling for duplicate key detection on save which could only be caused by an existing lock owned by someone else (based on the $or).

Show resolved Hide resolved src/Symfony/Component/Lock/Store/MongoDbStore.php Outdated
$key->reduceLifetime($this->initialTtl);
try {
$this->getCollection()->updateOne($filter, $update, $options);

This comment has been minimized.

Copy link
@GaneshChandrasekaran

GaneshChandrasekaran Oct 4, 2018

So you are doing an updateOne instead of insertOne here because you want to update the expiration time of a resource on same key when called multiple times correct?

This comment has been minimized.

Copy link
@kralos

kralos Oct 4, 2018

Author

Yes, if for some reason the user doesn't create a TTL Index (auto deleting index). A lock that gets left behind (expired) can be picked up and re-used for a different thread. The (or token equals) behavior is to allow successive calls to save to just work. For the later, see AbstractStoreTest::testSaveTwice, this is the intended behavior of all lock stores.

This comment has been minimized.

Copy link
@GaneshChandrasekaran

GaneshChandrasekaran Oct 5, 2018

Okay - that makes sense. Can we make the implementation such that if users don't call the ttl we set a default index which can be overridden with what user specifies? We don't want to leave the possibility of being able to leave documents behind in a collection and let it grow unknowingly.

This comment has been minimized.

Copy link
@kralos

kralos Oct 5, 2018

Author

Please don't get mixed up between:

a lock ttl i.e. public function __construct(Client $mongo, array $options, float $initialTtl = 300.0)

and a MongoDb TTL Index i.e. https://docs.mongodb.com/manual/core/index-ttl/

A MongoDb TTL Index is a database index which causes documents to be DELETED by the DBMS. Adding a MongoDB TTL index is a Database administration task. The MongoDbStore should NOT do this. It does however provide a method MongoDbStore::createTtlIndex allowing a programmer to automate the creation of this index in their deployment workflow. I personally would never use this method as it should only be called once when the database is created.

The responsibility of creating a MongoDB TTL Index is mentioned both on the constructor's phpdoc of this store and in the symfony documentation: https://github.com/symfony/symfony-docs/pull/9807/files

This comment has been minimized.

Copy link
@GaneshChandrasekaran

GaneshChandrasekaran Oct 5, 2018

Right, I was't confused between the two, sorry for using the wrong terminology there :)

This comment has been minimized.

Copy link
@kralos

kralos Oct 5, 2018

Author

so when you say:

if users don't call the ttl we set a default index

Are you talking about us automatically calling MongoDbStore::createTtlIndex?

Because this won't work on mongodb prior to version 2.2. Also we can't assume the application servers and DB servers are clock synced. Creating a TTL index when they are out can cause locks to be removed before they expire. In the worst case scenario, a database server could be on a different timezone to an application server.

If the user is running their locks across a stack spanning multiple data centers in different geographic regions they may decide to set a relatively high expireAfterSeconds. Someone running everything in 1 data center and running NTP might decide to set expireAfterSeconds = 0

This comment has been minimized.

Copy link
@GaneshChandrasekaran

GaneshChandrasekaran Oct 5, 2018

Yes that's what I suggesting to automatically call MongoDbStore::createTtlIndex. Okay, I am not sure if this is okay or not. Will let the core members of this component and symfony decide on this one :)

Have suggested on the PR how to clean up stale locks.

This comment has been minimized.

Copy link
@kralos

kralos Oct 15, 2018

Author

This has been added as the option gcProbability and I will update symfony/symfony-docs#9807 to mention increasing $initialTtl / $ttl to account for clock drift.

Show resolved Hide resolved src/Symfony/Component/Lock/Store/MongoDbStore.php Outdated
$token = $this->getUniqueToken($key);
$now = microtime(true);
$filter = array(

This comment has been minimized.

Copy link
@GaneshChandrasekaran

GaneshChandrasekaran Oct 4, 2018

I am not sure why you need the $or in the filter. Can this blocked be refactored as below for better readability?

        $filter = array(
            '_id' => (string) $key,
            'token' => $token,
            'expires_at' => array(
                '$lte' => $this->createDateTime($now),
            ),
        );

This comment has been minimized.

Copy link
@kralos

kralos Oct 4, 2018

Author

No;

We might want to pickup a leftover document from a previously expired instantiation of Key (same resource / _id) with a different token.

OR

We can overwrite our instantiation of Key's lock (AbstractStoreTest::testSaveTwice).

This comment has been minimized.

Copy link
@GaneshChandrasekaran

GaneshChandrasekaran Oct 5, 2018

Okay - thanks for mentioning that. IMO every store should clean up the key as well as the associated data with it on a release or expiration. Because, if a lock is only ever used once, it doesn't make sense to leave it present in the collection forever. I believe all the other stores also do this.
And we should also not override the AbstractStoreTest::testSaveTwice.

This comment has been minimized.

Copy link
@GaneshChandrasekaran

GaneshChandrasekaran Oct 5, 2018

I also don't understand the design:

We might want to pickup a leftover document from a previous instantiation of Key (same resource / _id) with a different token

How will the new process know that the lock resource is free to be acquired in a document with the key id already exists?

This comment has been minimized.

Copy link
@kralos

kralos Oct 5, 2018

Author

IMO every store should clean up the key as well as the associated data with it on a release or expiration

We do this on the MongoDBStore via the usage of a MongoDB TTL Index (see the phpdoc on the __construct and also https://github.com/symfony/symfony-docs/pull/9807/files

The implementation however is designed to be resilient in the event someone doesn't create a MongoDb TTL Index. It will begin to recycle documents ONLY if they are expired or they are owned by the current thread (this is why we have an $or in the query).

e.g.

$k1 = new \Symfony\Component\Lock\Key('my-resource');
$k2 = new \Symfony\Component\Lock\Key('my-resource'); // This has a different "token" to k1

$store->save($k1);

$store->save($k1);
// This is ok, we are re-saving k1 (scenario enforced by `AbstractStoreTest::testSaveTwice`)

$store->save($k2);
// This is NOT ok, k1 has the lock

sleep($lockTtl + 1);
// $k1 has now expired

$store->save($k2);
// This is ok, re-cycling the document created by k1 IF the developer ignored the recommendation to create a MongoDb TTL Index

This comment has been minimized.

Copy link
@GaneshChandrasekaran

GaneshChandrasekaran Oct 5, 2018

Okay this is making more sense to me, thanks for the explanation. Let me know if I am understanding this wrong:

In your above case, it is assumed that the client created a TTL index on the collection. There is a possibility that user doesn't explicitly call this. So will you run into the following scenario?

// TTL is not set on the collection
- $store->save($k1);  // it somehow creates the document with an expiration time in future, by default it is 300 seconds 
- // the client forgets to call release and the thread finishes processing
- // the collection still has `k1` with the original $token because the expiration value has no effect as TTL index was never created on Mongo DB through Database administration or by the client during DB setup once initially
- $store->save($k2); // this should never work because the token associated with `_id` never expires because no TTL Index was created

Or does not setting a TTL Index on the collection mean that, the document itself won't be deleted but the data ($token in our case) associated with _id (lock resource in our case) will get deleted but the document itself remains with the _id?

This comment has been minimized.

Copy link
@kralos

kralos Oct 5, 2018

Author

My scenario above does NOT assume a TTL index was created. The PHP will behave the same regardless of if the TTL Index exists. The only difference is if a document is left behind because PHP crashed and didn't get to remove the lock, the lock / document would persist forever (or until the next time another lock was acquired with the same resource id).

Think of a TTL Index like a cron that runs on an SQL database periodically:

DELETE FROM lock WHERE expires_at < (NOW() + :expireAfterSeconds);

It simply cleans up expired locks / documents

Assuming a TTL index is NOT created...

the collection still has k1 with the original $token because the expiration value has no effect as TTL index was never created on Mongo DB through Database administration or by the client during DB setup once initially

This is true, a document (a.k.a row in RDBMS world) will remain in the lock collection (a.k.a. table in RDBMS world) indefinitely.

$store->save($k2); // this should never work because the token associated with _id never expires because no TTL Index was created

This will work as soon as the expires_at <= $now ($k2 will overwritten into $k1's expired document), MongoDbStore::save will:

$filter = array(
    '_id' => (string) $key,
    '$or' => array(
        array(
            'token' => $token,
        ),
        array(
            'expires_at' => array(
                '$lte' => $this->createDateTime($now),
            ),
        ),
    ),
);

$update = array(
    '$set' => array(
        '_id' => (string) $key,
        'token' => $token,
        'expires_at' => $this->createDateTime($now + $this->initialTtl),
    ),
);

$options = array(
    'upsert' => true,
);

If however you tried to $store->save($k2); BEFORE expires_at <= $now (e.g. before $k1 has expired), the upsert would effectively attempt to INSERT which would cause a Mongo error E11000 - DuplicateKey.

This comment has been minimized.

Copy link
@GaneshChandrasekaran

GaneshChandrasekaran Oct 5, 2018

Okay makes sense now.

Show resolved Hide resolved src/Symfony/Component/Lock/Tests/Store/MongoDbStoreTest.php Outdated
/**
* {@inheritdoc}
*/
public function getStore()

This comment has been minimized.

Copy link
@GaneshChandrasekaran

GaneshChandrasekaran Oct 4, 2018

Can you add return type as StoreInterface please?

This comment has been minimized.

Copy link
@kralos

kralos Oct 4, 2018

Author

sure

This comment has been minimized.

Copy link
@kralos

kralos Oct 4, 2018

Author

@jderusse since AbstractStoreTest doesn't yet specify the return type should we go ahead an upgrade all missing return types from Symfony\Component\Lock? What's the policy on doing this at the moment?

This comment has been minimized.

Copy link
@GaneshChandrasekaran

GaneshChandrasekaran Oct 5, 2018

I would suggest only making changes for the stores implemented in this PR. We can open a separate issue/PR to fix in other places.

Show resolved Hide resolved src/Symfony/Component/Lock/Store/MongoDbStore.php Outdated
),
array(
'expires_at' => array(
'$lte' => $this->createDateTime($now),

This comment has been minimized.

Copy link
@GaneshChandrasekaran

GaneshChandrasekaran Oct 4, 2018

  1. why do you need the $lte
  2. why not $this->createDateTime($now + $this->initialTtl) instead?

This comment has been minimized.

Copy link
@kralos

kralos Oct 4, 2018

Author

Sorry, I don't really follow... This is a filter looking for locks that have expired.

This comment has been minimized.

Copy link
@GaneshChandrasekaran

GaneshChandrasekaran Oct 5, 2018

I am not familiar with mongodb so just wanted to understand what was happening here. :)

This comment has been minimized.

Copy link
@kralos

kralos Oct 5, 2018

Author
UPDATE OR INSERT lock
SET
    id = :key,
    token = :token,
    expires_at = :newExpiry
WHERE
    id = :key
    AND (
        token = :key
        OR expires_at <= :now
    )
:key = (string)$key;
:token = $this->getUniqueToken($key);
:newExpiry = $this->createDateTime($now + $this->initialTtl);
:now = new \DateTime('now');

Does this explain it? (Assuming UPDATE OR INSERT is a real thing and it's ACID compliant being 1 statement).

This comment has been minimized.

Copy link
@kralos

kralos Oct 5, 2018

Author

First mongo will attempt to locate an existing document WHERE <conditions> and if it finds one it will be updated with SET <fields>.

Otherwise it will attempt to INSERT (id, token, expires_at) VALUES (<fields>)

The INSERT could fail if there's a duplicate key exception on id.

In MongoDb world _id is a PRIMARY KEY

This comment has been minimized.

Copy link
@GaneshChandrasekaran

GaneshChandrasekaran Oct 5, 2018

The logic makes more sense now. Thanks for explaining.

/**
* {@inheritdoc}
*
* @throws LockStorageException if failed to save to storage (fallback exception)

This comment has been minimized.

Copy link
@GaneshChandrasekaran

GaneshChandrasekaran Oct 4, 2018

From the documentation, it seems like LockStorageException should only be used when getting an error trying to manipulate an already existing resource in the store.

/cc - @jderusse in case he has better insight between the usage of LockStorageException or LockAcquiringException

This comment has been minimized.

Copy link
@kralos

kralos Oct 4, 2018

Author

I'm just looking at Lock::acquire and Lock::refresh; both have a catch (\Exception $e) re-throwing as LockAcquiringException. I have therefore just removed my } catch (Exception $e) {'s in MongoDbStore::save and MongoDbStore::putOffExpiration to allow Lock to maintain exception consistency.

* {@inheritdoc}
*
* @throws LockStorageException if failed to save to storage (fallback exception)
* @throws LockExpiredException when save is called on an expired lock

This comment has been minimized.

Copy link
@GaneshChandrasekaran

GaneshChandrasekaran Oct 4, 2018

The lock.php already does this check so I believe this can be removed.

This comment has been minimized.

Copy link
@kralos

kralos Oct 4, 2018

Author

I have removed it from my implementation, @jderusse do you think we should do the same to all other stores? They all currently seem to check this.

This comment has been minimized.

Copy link
@kralos

kralos Oct 4, 2018

Author

actually I can't because ExpiringStoreTestTrait::testAbortAfterExpiration expects the store to check this. I have re-instated it however if Jérémy wants to move responsibility of this check from the Stores up to Lock we could (in a separate ticket) change all stores and the test.

This comment has been minimized.

Copy link
@GaneshChandrasekaran

GaneshChandrasekaran Oct 5, 2018

Okay makes sense. We can take care of this in separate ticket. I believe we will also revisit in future on the store interface to clearly specify contract depending on what kind of store we are implementing.

This comment has been minimized.

Copy link
@kralos

@kralos kralos force-pushed the kralos:27345-lock-mongodb branch from 0b4abbb to c44b211 Oct 4, 2018

$token = $this->getUniqueToken($key);
$now = microtime(true);
$filter = array(

This comment has been minimized.

Copy link
@GaneshChandrasekaran

GaneshChandrasekaran Oct 5, 2018

Okay - thanks for mentioning that. IMO every store should clean up the key as well as the associated data with it on a release or expiration. Because, if a lock is only ever used once, it doesn't make sense to leave it present in the collection forever. I believe all the other stores also do this.
And we should also not override the AbstractStoreTest::testSaveTwice.

* {@inheritdoc}
*
* @throws LockStorageException if failed to save to storage (fallback exception)
* @throws LockExpiredException when save is called on an expired lock

This comment has been minimized.

Copy link
@GaneshChandrasekaran

GaneshChandrasekaran Oct 5, 2018

Okay makes sense. We can take care of this in separate ticket. I believe we will also revisit in future on the store interface to clearly specify contract depending on what kind of store we are implementing.

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

This comment has been minimized.

Copy link
@GaneshChandrasekaran

GaneshChandrasekaran Oct 5, 2018

Okay - that makes sense. Can we make the implementation such that if users don't call the ttl we set a default index which can be overridden with what user specifies? We don't want to leave the possibility of being able to leave documents behind in a collection and let it grow unknowingly.

$token = $this->getUniqueToken($key);
$now = microtime(true);
$filter = array(

This comment has been minimized.

Copy link
@GaneshChandrasekaran

GaneshChandrasekaran Oct 5, 2018

I also don't understand the design:

We might want to pickup a leftover document from a previous instantiation of Key (same resource / _id) with a different token

How will the new process know that the lock resource is free to be acquired in a document with the key id already exists?

/**
* @return int
*/
protected function getClockDelay()

This comment has been minimized.

Copy link
@GaneshChandrasekaran

GaneshChandrasekaran Oct 5, 2018

getClockDelay(): int

This comment has been minimized.

Copy link
@kralos

kralos Oct 5, 2018

Author

added

/**
* {@inheritdoc}
*/
public function exists(Key $key)

This comment has been minimized.

Copy link
@GaneshChandrasekaran

GaneshChandrasekaran Oct 5, 2018

exists(Key $key): bool

This comment has been minimized.

Copy link
@kralos

kralos Oct 5, 2018

Author

added, initially I wasn't sure if I should be doing this kind of stuff since the parent doesn't do it... but it seems to work.

Show resolved Hide resolved src/Symfony/Component/Lock/Store/MongoDbStore.php Outdated
),
array(
'expires_at' => array(
'$lte' => $this->createDateTime($now),

This comment has been minimized.

Copy link
@GaneshChandrasekaran

GaneshChandrasekaran Oct 5, 2018

I am not familiar with mongodb so just wanted to understand what was happening here. :)

Show resolved Hide resolved src/Symfony/Component/Lock/Store/MongoDbStore.php Outdated

@kralos kralos force-pushed the kralos:27345-lock-mongodb branch from c44b211 to 8a51f9f Oct 5, 2018

@kralos

This comment has been minimized.

Copy link
Author

commented Oct 5, 2018

@GaneshChandrasekaran I've responded to all of your comments, can you please resolve every discussion you are happy with?

@kralos kralos force-pushed the kralos:27345-lock-mongodb branch from 8a51f9f to 6999955 Oct 5, 2018

@GaneshChandrasekaran

This comment has been minimized.

Copy link

commented Oct 5, 2018

Thanks for your patience and answering all my questions @kralos

The only concern that I have left is, this implementation leaves with a possibility of leaving behind stale locks in the DB if an application encounters a crash or some other failure or if the version of mongodb is older than 2.2. This can be bad if you have a high throughput application that requires a lock only once to avoid duplicate processing or something.

I am thinking maybe it should also do something similar to PDOStore which seems to clean up stale locks at random intervals
https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Lock/Store/PdoStore.php#L143

try {
$this->getCollection()->updateOne($filter, $update, $options);
} catch (WriteException $e) {
$writeErrors = $e->getWriteResult()->getWriteErrors();

This comment has been minimized.

Copy link
@GaneshChandrasekaran

GaneshChandrasekaran Oct 6, 2018

You could extract this logic into a function to follow DRY principle :)

This comment has been minimized.

Copy link
@kralos

kralos Oct 8, 2018

Author

moved

/**
* {@inheritdoc}
*/
public function waitAndSave(Key $key)

This comment has been minimized.

Copy link
@GaneshChandrasekaran

GaneshChandrasekaran Oct 6, 2018

Can you add a test for this to make sure the logic to throw exception works?

This comment has been minimized.

Copy link
@kralos

kralos Oct 8, 2018

Author

added

public function waitAndSave(Key $key)
{
throw new NotSupportedException(sprintf(
'The store "%s" does not supports blocking locks.',

This comment has been minimized.

Copy link
@GaneshChandrasekaran

GaneshChandrasekaran Oct 6, 2018

supports -> support ?

This comment has been minimized.

Copy link
@kralos

kralos Oct 8, 2018

Author

fixed

@kralos

This comment has been minimized.

Copy link
Author

commented Oct 8, 2018

Thanks for your patience and answering all my questions @kralos

The only concern that I have left is, this implementation leaves with a possibility of leaving behind stale locks in the DB if an application encounters a crash or some other failure or if the version of mongodb is older than 2.2. This can be bad if you have a high throughput application that requires a lock only once to avoid duplicate processing or something.

I am thinking maybe it should also do something similar to PDOStore which seems to clean up stale locks at random intervals
https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Lock/Store/PdoStore.php#L143

I've had a look at PdoStore's implementation and I have some concerns with it:

  1. The expiry time is using the DBMS's clock for cleanup, what if the DBMS clock differs from the application server? It would be possible to use a timestamp generated from PHP for the WHERE clause. Shouldn't we avoid this where possible?
  2. What about clock drift? If there are multiple servers spanning geographic regions should the developer be allowed to specify an acceptable drift (seconds) allowing cleanup to happen slightly after expiry? e.g. MongoDbStore::createTtlIndex(int $expireAfterSeconds = 0) The $expireAfterSeconds allows the developer / sysadmin to specify acceptable delay between the expiry and deletion.

I did also have a concern about people who run servers on different timezones however I'm not sure if we care about these people... :-p

*
* @author Joe Bennett <joe@assimtech.com>
*/
class MongoDbStore implements StoreInterface

This comment has been minimized.

Copy link
@GaneshChandrasekaran

GaneshChandrasekaran Oct 14, 2018

Starting a thread here:

Thanks for your patience and answering all my questions @kralos
The only concern that I have left is, this implementation leaves with a possibility of leaving behind stale locks in the DB if an application encounters a crash or some other failure or if the version of mongodb is older than 2.2. This can be bad if you have a high throughput application that requires a lock only once to avoid duplicate processing or something.
I am thinking maybe it should also do something similar to PDOStore which seems to clean up stale locks at random intervals
https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Lock/Store/PdoStore.php#L143

I've had a look at PdoStore's implementation and I have some concerns with it:

  1. The expiry time is using the DBMS's clock for cleanup, what if the DBMS clock differs from the application server? It would be possible to use a timestamp generated from PHP for the WHERE clause. Shouldn't we avoid this where possible?
  2. What about clock drift? If there are multiple servers spanning geographic regions should the developer be allowed to specify an acceptable drift (seconds) allowing cleanup to happen slightly after expiry? e.g. MongoDbStore::createTtlIndex(int $expireAfterSeconds = 0) The $expireAfterSeconds allows the developer / sysadmin to specify acceptable delay between the expiry and deletion.

I did also have a concern about people who run servers on different timezones however I'm not sure if we care about these people... :-p

This is a good question. Looks like @jderusse is solving for the problem by expecting clients to give a higher TTL from the beginning and it also assumes that the client and server locks are synchronized. I don't particularly see any problem with you generating a timestamp from PHP side to solve this problem.

This comment has been minimized.

Copy link
@kralos

kralos Oct 15, 2018

Author

Assuming we are going to suggest a higher TTL to account for clock drift I have implemented automatic mongodb TTL Index creation. I did this using a probability modifier like php session / PDO store however due to the incompatibility with MongoDB prior to 2.2 I have also implemented mongodb version detection and reverted to a delete expired documents for older servers. I have still allowed for a developer to set their probability to 0.0 (effectively disabling the automatic TTL creation) in case they wish to manually manage the creation of their TTL Index. Let me know what you think?

This comment has been minimized.

Copy link
@GaneshChandrasekaran

GaneshChandrasekaran Oct 15, 2018

This seems fine to me. Thanks!

This comment has been minimized.

Copy link
@jderusse

jderusse Oct 15, 2018

Contributor

The current implementation of PdoStore always use the SGDB internal clock. This would avoid collision when 2 servers are not time synced. I suggest to the the same here.

From an other hand, the documentation recommend to take clock drift into account when diffining the TTL (https://github.com/symfony/symfony-docs/pull/9875/files#diff-1dc6462c0bc00fcdc7cba99d2f13e400R499)

This comment has been minimized.

Copy link
@kralos

kralos Oct 15, 2018

Author

The equivalent of SQL: NOW() wasn't introduced until MongoDB 2.6 for UPDATE: https://jira.mongodb.org/browse/SERVER-10911

There is no equivalent of DATE_ADD() for an update.

I looked into doing it this way first because I have concerns about PHP $ttl expiry not being strictly adhered to (due to clock drift) however it's not possible in MongoDB. In light of this I think it's down to setting a higher $initialTtl / $ttl if a developer needs to account for high drift.

This comment has been minimized.

Copy link
@kralos

kralos Oct 23, 2018

Author

@jderusse is the current implementation ok with you since it's not possible to do all time from the DB's clock? I'll be sure to document this on symfony/symfony-docs#9807

This comment has been minimized.

Copy link
@jderusse

jderusse Mar 24, 2019

Contributor

Given mongoDb don't provide methods to manipulate dates on upsert. I'm fine with using the APP date, and warning the user in the documentation.

@GaneshChandrasekaran
Copy link

left a comment

Looks good to me :)

Pinging @jderusse for his review from here.

*
* @author Joe Bennett <joe@assimtech.com>
*/
class MongoDbStore implements StoreInterface

This comment has been minimized.

Copy link
@GaneshChandrasekaran

GaneshChandrasekaran Oct 15, 2018

This seems fine to me. Thanks!

/**
* @return bool
*/
private function isDuplicateKeyException(

This comment has been minimized.

Copy link
@GaneshChandrasekaran

GaneshChandrasekaran Oct 15, 2018

these can be on single line

This comment has been minimized.

Copy link
@kralos

kralos Mar 28, 2019

Author

updated

* Create a TTL index to automatically remove expired locks.
*
* If the createTtlIndexProbability option is set higher than 0.0
* (defaults to 0.001); this will be called automatically on self::save().

This comment has been minimized.

Copy link
@jderusse

jderusse Oct 15, 2018

Contributor

there will be a chance to be called...

This comment has been minimized.

Copy link
@kralos

kralos Oct 18, 2018

Author

updated

@jderusse
Copy link
Contributor

left a comment

Few comments, the big issue is the use of application's clock instead of Database's one

* Options:
* database: The name of the database [required]
* collection: The name of the collection [default: lock]
* createTtlIndexProbability: Should a TTL Index be created expressed as a probability from 0.0 to 1.0 [default: 0.001]

This comment has been minimized.

Copy link
@jderusse

jderusse Oct 15, 2018

Contributor

This option is used both for creating index and removing manually expired locks (depends on the version).
What's about gcProbablity like in PdoStore?

Moreover, I wonder if ti make send to create several time the TTL index...

This comment has been minimized.

Copy link
@kralos

kralos Oct 18, 2018

Author

I changed to gcProbability.

Re creating the TTL Index several times, in Mongo most actions are usually idempotent. There used to be a method called ensureIndex however it was deprecated in favour of just calling createIndex see: https://docs.mongodb.com/manual/reference/method/db.collection.ensureIndex/

try {
$store->waitAndSave($key);
$this->fail('The store shouldn\'t support waitAndSave');
} catch (NotSupportedException $e) {

This comment has been minimized.

This comment has been minimized.

Copy link
@kralos

kralos Oct 18, 2018

Author

updated

'_id' => (string) $key,
'token' => $this->getUniqueToken($key),
'expires_at' => array(
'$gte' => $this->createDateTime(),

This comment has been minimized.

Copy link
@jderusse

jderusse Oct 15, 2018

Contributor

You have to define whether equals means locked or available. This statement conflict with $lte in save

This comment has been minimized.

Copy link
@kralos

kralos Oct 18, 2018

Author

fixed

'_id' => (string) $key,
'token' => $this->getUniqueToken($key),
'expires_at' => array(
'$gte' => $this->createDateTime($now),

This comment has been minimized.

Copy link
@jderusse

jderusse Oct 15, 2018

Contributor

should be token = $token OR expires_at <= now() Look at PdoStore for consistency

This comment has been minimized.

Copy link
@kralos

kralos Oct 18, 2018

Author

I have consolidated save and putOffExpiration into a common method since they are now the same DB statement

*
* @author Joe Bennett <joe@assimtech.com>
*/
class MongoDbStore implements StoreInterface

This comment has been minimized.

Copy link
@jderusse

jderusse Oct 15, 2018

Contributor

The current implementation of PdoStore always use the SGDB internal clock. This would avoid collision when 2 servers are not time synced. I suggest to the the same here.

From an other hand, the documentation recommend to take clock drift into account when diffining the TTL (https://github.com/symfony/symfony-docs/pull/9875/files#diff-1dc6462c0bc00fcdc7cba99d2f13e400R499)

*/
public function waitAndSave(Key $key)
{
throw new NotSupportedException(sprintf(

This comment has been minimized.

Copy link
@jderusse

jderusse Oct 15, 2018

Contributor

use a single line for consistency

This comment has been minimized.

Copy link
@kralos

kralos Oct 18, 2018

Author

fixed

/**
* @return bool
*/
private function isDuplicateKeyException(

This comment has been minimized.

Copy link
@jderusse

jderusse Oct 15, 2018

Contributor

Move after public methods

This comment has been minimized.

Copy link
@kralos

kralos Oct 18, 2018

Author

moved

@kralos kralos force-pushed the kralos:27345-lock-mongodb branch 4 times, most recently from faa6e1c to 692155e Oct 15, 2018

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 24, 2019

@jderusse How can we move this one forward?

@jderusse
Copy link
Contributor

left a comment

Few minor comment.

Thank's for the reminder @fabpot

*
* @see http://docs.mongodb.org/manual/tutorial/expire-data/
*
* @return string The name of the created index

This comment has been minimized.

Copy link
@jderusse

jderusse Mar 24, 2019

Contributor

no needed to return it IMHO

This comment has been minimized.

Copy link
@kralos

kralos Mar 28, 2019

Author

removed the return

Show resolved Hide resolved src/Symfony/Component/Lock/Store/MongoDbStore.php Outdated
/**
* @return Manager
*/
private function getManager(): Manager

This comment has been minimized.

Copy link
@jderusse

jderusse Mar 24, 2019

Contributor

Can be removed. This method is called once in the class: inside the getDatabaseVersion inside a if cached missed

This comment has been minimized.

Copy link
@kralos

kralos Mar 28, 2019

Author

removed

* @throws MongoInvalidArgumentException for parameter/option parsing errors
* @throws DriverRuntimeException for other driver errors (e.g. connection errors)
*/
public function createTtlIndex(int $expireAfterSeconds = 0): string

This comment has been minimized.

Copy link
@jderusse

jderusse Mar 24, 2019

Contributor

This method should be either

  • public: must be called by user (and disabled the GC logic when version >= 2.2)
  • private: automatically called by GC

This comment has been minimized.

Copy link
@kralos

kralos Mar 28, 2019

Author

My intent was to make default behaviour to create a TTL Index with a gcProbablity default of 0.001. This is to prevent orphan locks for anyone who uses this store without reading docs or changing any config.

If someone doesn't want this to happen (and they read the docs), I left the createTtlIndex method public allowing someone to configure gcProbablity = 0.0 and hook createTtlIndex in their deployment or database set up scripts.

See constructor docs:

 * .... If you prefer to create your TTL Index
 * manually you can set gcProbablity to 0.0 and optionally leverage
 * self::createTtlIndex(int $expireAfterSeconds = 0).

It's my opinion setting TTL index automatically is dangerous (if someone has differing clocks between DBMS and App) as well as an unnecessary expense. I would like to make a recommendation in symfony/symfony-docs#9807 to set gcProbablity = 0.0 and manually create the TTLIndex.

If you disagree this this approach, which way do you think is the better way to go?

This comment has been minimized.

Copy link
@jderusse

jderusse Mar 28, 2019

Contributor

👍 I forgot the part about GC=0 and creting the index manually. Sound's good to me

'expireAfterSeconds' => $expireAfterSeconds,
);
return $this->getCollection()->createIndex($keys, $options);

This comment has been minimized.

Copy link
@jderusse

jderusse Mar 24, 2019

Contributor

What if the method is called several time (like you do in GC)? any impact on existing data or performances?

This comment has been minimized.

Copy link
@kralos

kralos Mar 28, 2019

Author

createIndex is idempotent in MongoDb https://docs.mongodb.com/manual/indexes/#create-an-index

The db.collection.createIndex method only creates an index if an index of the same specification does not already exist.

There used to be an alias called ensureIndex which was deprecated in favour of createIndex (IMO they should have deprecated createIndex).

https://docs.mongodb.com/manual/reference/method/db.collection.ensureIndex/

*
* @author Joe Bennett <joe@assimtech.com>
*/
class MongoDbStore implements StoreInterface

This comment has been minimized.

Copy link
@jderusse

jderusse Mar 24, 2019

Contributor

Given mongoDb don't provide methods to manipulate dates on upsert. I'm fine with using the APP date, and warning the user in the documentation.

@kralos kralos force-pushed the kralos:27345-lock-mongodb branch 2 times, most recently from a017cea to 1f4d601 Mar 28, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

Tests are broken.

Joe Bennett

@kralos kralos force-pushed the kralos:27345-lock-mongodb branch from 02e826e to 9c04639 Mar 29, 2019

@kralos

This comment has been minimized.

Copy link
Author

commented Mar 29, 2019

@fabpot composer update is intermittently breaking AppVeyor

php composer.phar update --no-progress --no-suggest --ansi
Loading composer repositories with package information
Updating dependencies (including require-dev)
Restricting packages listed in "symfony/symfony" to ">=4.2"
Failed to decode response: Failed to decode zlib stream
Retrying with degraded mode, check https://getcomposer.org/doc/articles/troubleshooting.md#degraded-mode for more info
Command exited with code -1073741819
@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

Thank you @kralos.

@fabpot fabpot merged commit 9c04639 into symfony:master Mar 29, 2019

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 29, 2019

feature #27648 [Lock] Added MongoDBStore (Joe Bennett)
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

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Apr 1, 2019

minor #9807 #27345 Added MongoDbStore Lock Store documentation (Joe B…
…ennett)

This PR was merged into the master branch.

Discussion
----------

#27345 Added MongoDbStore Lock Store documentation

Added support for MongoDbStore Lock Store, See symfony/symfony#27648

Commits
-------

a641baa #27345 Added docs for Symfony\Component\Lock\Store\MongoDbStore

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.