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.

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).

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?

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.

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.

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

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

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

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 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.

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),
            ),
        );

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).

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.

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?

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

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?

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.

Okay makes sense now.

Can you add return type as StoreInterface please?

sure

@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?

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.

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

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

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

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).

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

The logic makes more sense now. Thanks for explaining.

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

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.

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

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.

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.

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.

See #28779

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.

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.

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.

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?

getClockDelay(): int

added

exists(Key $key): bool

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.

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

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.

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 seems fine to me. Thanks!

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)

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.

@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

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.

This seems fine to me. Thanks!

these can be on single line

updated

there will be a chance to be called...

updated

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...

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/

use https://phpunit.de/manual/6.5/en/appendixes.annotations.html#appendixes.annotations.expectedException to assert that an exception is thorwn

updated

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