Skip to content

PHPORM-99 Enable TTL index to auto-purge of expired cache and lock items #2891

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

Merged
merged 7 commits into from
Apr 23, 2024
Merged

PHPORM-99 Enable TTL index to auto-purge of expired cache and lock items #2891

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c7fce86
Enable TTL index to auto-purge of expired cache and lock items
GromNaN Apr 22, 2024
087e8fd
Simplify constructor arguments of MongoLock
GromNaN Apr 22, 2024
e0da8e9
Remove useless expiration condition in cache increment
GromNaN Apr 22, 2024
4a5a3d8
Rename expiration field to expires_at for naming consistency
GromNaN Apr 23, 2024
33eb655
Validate lottery value
GromNaN Apr 23, 2024
42e890c
Revert lock acquire to use findOneAndUpdate
GromNaN Apr 23, 2024
8d02b31
Fix test using UTCDateTime
GromNaN Apr 23, 2024
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
48 changes: 27 additions & 21 deletions src/Cache/MongoLock.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
namespace MongoDB\Laravel\Cache;

use Illuminate\Cache\Lock;
use Illuminate\Support\Carbon;
use MongoDB\BSON\UTCDateTime;
use MongoDB\Laravel\Collection;
use MongoDB\Operation\FindOneAndUpdate;
use Override;
Expand All @@ -14,20 +16,18 @@ final class MongoLock extends Lock
/**
* Create a new lock instance.
*
* @param Collection $collection The MongoDB collection
* @param string $name Name of the lock
* @param int $seconds Time-to-live of the lock in seconds
* @param string|null $owner A unique string that identifies the owner. Random if not set
* @param array $lottery The prune probability odds
* @param int $defaultTimeoutInSeconds The default number of seconds that a lock should be held
* @param Collection $collection The MongoDB collection
* @param string $name Name of the lock
* @param int $seconds Time-to-live of the lock in seconds
* @param string|null $owner A unique string that identifies the owner. Random if not set
* @param array{int, int} $lottery Probability [chance, total] of pruning expired cache items. Set to [0, 0] to disable
*/
public function __construct(
private readonly Collection $collection,
string $name,
int $seconds,
?string $owner = null,
private readonly array $lottery = [2, 100],
private readonly int $defaultTimeoutInSeconds = 86400,
Copy link
Member

Choose a reason for hiding this comment

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

Why was this removed entirely? It looks like it previously served a purpose if $seconds was zero. Now, specifying zero for $seconds would cause locks to immediately expire.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since $defaultTimeoutInSeconds was only a default value, I moved the processing to MongoStore::lock. It is not possible to disable lock expiration.

) {
parent::__construct($name, $seconds, $owner);
}
Expand All @@ -41,11 +41,11 @@ public function acquire(): bool
// or it is already owned by the same lock instance.
$isExpiredOrAlreadyOwned = [
'$or' => [
['$lte' => ['$expiration', $this->currentTime()]],
['$lte' => ['$expiration', $this->getUTCDateTime()]],
['$eq' => ['$owner', $this->owner]],
],
];
$result = $this->collection->findOneAndUpdate(
$result = $this->collection->updateOne(
['_id' => $this->name],
[
[
Expand All @@ -60,7 +60,7 @@ public function acquire(): bool
'expiration' => [
'$cond' => [
'if' => $isExpiredOrAlreadyOwned,
'then' => $this->expiresAt(),
'then' => $this->getUTCDateTime($this->seconds),
'else' => '$expiration',
],
],
Expand All @@ -74,11 +74,11 @@ public function acquire(): bool
],
);

if (random_int(1, $this->lottery[1]) <= $this->lottery[0]) {
$this->collection->deleteMany(['expiration' => ['$lte' => $this->currentTime()]]);
if (! empty($this->lottery[0]) && random_int(1, $this->lottery[1]) <= $this->lottery[0]) {
Copy link
Member

Choose a reason for hiding this comment

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

Does anything enforce that $this->lottery is a tuple? This only checks that the first element exists and is non-empty, but there is no check for a second element or both being integers.

Perhaps that should be done in the constructor.

I assume you intentionally decided not to make $lottery nullable as a means to disable it.

Copy link
Member Author

Choose a reason for hiding this comment

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

It seemed to me that the "zero chances on whatever" value was explicit enough to say that we wanted to disable this mechanism.

$this->collection->deleteMany(['expiration' => ['$lte' => $this->getUTCDateTime()]]);
}

return $result['owner'] === $this->owner;
return $result->getModifiedCount() > 0 || $result->getUpsertedCount() > 0;
Copy link
Member

Choose a reason for hiding this comment

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

Wasn't this logic problematic in the original PR? Are you now relying on the UTCDateTime always changing due to sub-second precision?

Copy link
Member Author

Choose a reason for hiding this comment

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

No problem with the previous logic. I said to myself that it was impossible to have the same lock with the same owner acquired in the same microsecond. But I've reverted to that, so you don't have to worry.

}

/**
Expand Down Expand Up @@ -107,6 +107,17 @@ public function forceRelease(): void
]);
}

/** Creates a TTL index that automatically deletes expired objects. */
public function createTTLIndex(): void
Copy link
Member

Choose a reason for hiding this comment

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

I intentionally didn't suggest using a TTL index in #2877 because it seemed redundant with the $lottery constructor argument that remained in place.

Given that these are just public methods on the MongoLock and MongoStore classes, how do you expect users to interact with them? My understanding is that both classes would typically be used while handling a request, and this method seems more like schema setup. It makes sense for MongoDB, of course, but doesn't fit very well into whatever common API Laravel intended for these classes.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is an additional feature that should be documented as I have described. The methods will probably be called in a migration. I think that for applications with a significant volume of traffic, using TTL indexes will have a positive impact on performance without having to search for the correct lottery value.

{
$this->collection->createIndex(
// UTCDateTime field that holds the expiration date
['expiration' => 1],
// Delay to remove items after expiration
['expireAfterSeconds' => 0],
);
}

/**
* Returns the owner value written into the driver for this lock.
*/
Expand All @@ -116,19 +127,14 @@ protected function getCurrentOwner(): ?string
return $this->collection->findOne(
[
'_id' => $this->name,
'expiration' => ['$gte' => $this->currentTime()],
'expiration' => ['$gte' => $this->getUTCDateTime()],
],
['projection' => ['owner' => 1]],
)['owner'] ?? null;
}

/**
* Get the UNIX timestamp indicating when the lock should expire.
*/
private function expiresAt(): int
private function getUTCDateTime(int $additionalSeconds = 0): UTCDateTime
{
$lockTimeout = $this->seconds > 0 ? $this->seconds : $this->defaultTimeoutInSeconds;

return $this->currentTime() + $lockTimeout;
return new UTCDateTime(Carbon::now()->addSeconds($additionalSeconds));
Copy link
Member

Choose a reason for hiding this comment

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

What allows UTCDateTime to be constructed from a Carbon instance? Is this relying on Carbon's string representation?

I looked at the Addition and Subtraction docs for Carbon and the strings in the examples don't look correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

The Carbon class extends DateTime, and the UTCDateTime constructor accepts any DateTimeInterface. There is no string transformation.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I missed Carbon's inheritance. SGTM!

}
}
44 changes: 29 additions & 15 deletions src/Cache/MongoStore.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@
use Illuminate\Cache\RetrievesMultipleKeys;
use Illuminate\Contracts\Cache\LockProvider;
use Illuminate\Contracts\Cache\Store;
use Illuminate\Support\InteractsWithTime;
use Illuminate\Support\Carbon;
use MongoDB\BSON\UTCDateTime;
use MongoDB\Laravel\Collection;
use MongoDB\Laravel\Connection;
use MongoDB\Operation\FindOneAndUpdate;
Expand All @@ -20,7 +21,6 @@

final class MongoStore implements LockProvider, Store
{
use InteractsWithTime;
// Provides "many" and "putMany" in a non-optimized way
use RetrievesMultipleKeys;

Expand All @@ -34,7 +34,7 @@ final class MongoStore implements LockProvider, Store
* @param string $prefix Prefix for the name of cache items
* @param Connection|null $lockConnection The MongoDB connection to use for the lock, if different from the cache connection
* @param string $lockCollectionName Name of the collection where locks are stored
* @param array{int, int} $lockLottery Probability [chance, total] of pruning expired cache items
* @param array{int, int} $lockLottery Probability [chance, total] of pruning expired cache items. Set to [0, 0] to disable
* @param int $defaultLockTimeoutInSeconds Time-to-live of the locks in seconds
*/
public function __construct(
Expand Down Expand Up @@ -62,10 +62,9 @@ public function lock($name, $seconds = 0, $owner = null): MongoLock
return new MongoLock(
($this->lockConnection ?? $this->connection)->getCollection($this->lockCollectionName),
$this->prefix . $name,
$seconds,
$seconds ?: $this->defaultLockTimeoutInSeconds,
$owner,
$this->lockLottery,
$this->defaultLockTimeoutInSeconds,
);
}

Expand Down Expand Up @@ -95,7 +94,7 @@ public function put($key, $value, $seconds): bool
[
'$set' => [
'value' => $this->serialize($value),
'expiration' => $this->currentTime() + $seconds,
'expiration' => $this->getUTCDateTime($seconds),
],
],
[
Expand All @@ -116,6 +115,8 @@ public function put($key, $value, $seconds): bool
*/
public function add($key, $value, $seconds): bool
{
$isExpired = ['$lte' => ['$expiration', $this->getUTCDateTime()]];

$result = $this->collection->updateOne(
[
'_id' => $this->prefix . $key,
Expand All @@ -125,15 +126,15 @@ public function add($key, $value, $seconds): bool
'$set' => [
'value' => [
'$cond' => [
'if' => ['$lte' => ['$expiration', $this->currentTime()]],
'if' => $isExpired,
'then' => $this->serialize($value),
'else' => '$value',
],
],
'expiration' => [
'$cond' => [
'if' => ['$lte' => ['$expiration', $this->currentTime()]],
'then' => $this->currentTime() + $seconds,
'if' => $isExpired,
'then' => $this->getUTCDateTime($seconds),
'else' => '$expiration',
],
],
Expand Down Expand Up @@ -163,7 +164,7 @@ public function get($key): mixed
return null;
}

if ($result['expiration'] <= $this->currentTime()) {
if ($result['expiration'] <= $this->getUTCDateTime()) {
$this->forgetIfExpired($key);

return null;
Expand All @@ -181,12 +182,9 @@ public function get($key): mixed
#[Override]
public function increment($key, $value = 1): int|float|false
{
$this->forgetIfExpired($key);

$result = $this->collection->findOneAndUpdate(
[
'_id' => $this->prefix . $key,
'expiration' => ['$gte' => $this->currentTime()],
Copy link
Member

Choose a reason for hiding this comment

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

By removing this criteria and the preceding call to forgetIfExpired(), you're potentially incrementing an expired document. Is that a concern?

Since the TTL index is optional, I wouldn't have expected this to change.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've removed the previous forgetIfExpired in order to avoid useless command. A nominal usage must be a single command.

Even if the expired item is incremented here, it is deleted just after.

],
[
'$inc' => ['value' => $value],
Expand All @@ -200,7 +198,7 @@ public function increment($key, $value = 1): int|float|false
return false;
}

if ($result['expiration'] <= $this->currentTime()) {
if ($result['expiration'] <= $this->getUTCDateTime()) {
$this->forgetIfExpired($key);

return false;
Expand Down Expand Up @@ -257,7 +255,7 @@ public function forgetIfExpired($key): bool
{
$result = $this->collection->deleteOne([
'_id' => $this->prefix . $key,
'expiration' => ['$lte' => $this->currentTime()],
'expiration' => ['$lte' => $this->getUTCDateTime()],
]);

return $result->getDeletedCount() > 0;
Expand All @@ -275,6 +273,17 @@ public function getPrefix(): string
return $this->prefix;
}

/** Creates a TTL index that automatically deletes expired objects. */
public function createTTLIndex(): void
{
$this->collection->createIndex(
// UTCDateTime field that holds the expiration date
['expiration' => 1],
// Delay to remove items after expiration
['expireAfterSeconds' => 0],
);
}

private function serialize($value): string|int|float
{
// Don't serialize numbers, so they can be incremented
Expand All @@ -293,4 +302,9 @@ private function unserialize($value): mixed

return unserialize($value);
}

private function getUTCDateTime(int $additionalSeconds = 0): UTCDateTime
{
return new UTCDateTime(Carbon::now()->addSeconds($additionalSeconds));
}
}
21 changes: 16 additions & 5 deletions tests/Cache/MongoCacheStoreTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
use Illuminate\Support\Carbon;
use Illuminate\Support\Facades\Cache;
use Illuminate\Support\Facades\DB;
use MongoDB\BSON\UTCDateTime;
use MongoDB\Laravel\Tests\TestCase;

use function assert;
Expand Down Expand Up @@ -200,32 +201,42 @@ public function testIncrementDecrement()
$this->assertFalse($store->increment('foo', 5));
}

protected function getStore(): Repository
public function testTTLIndex()
{
$store = $this->getStore();
$store->createTTLIndex();

// TTL index remove expired items asynchronously, this test would be very slow
$indexes = DB::connection('mongodb')->getCollection($this->getCacheCollectionName())->listIndexes();
$this->assertCount(2, $indexes);
}

private function getStore(): Repository
{
$repository = Cache::store('mongodb');
assert($repository instanceof Repository);

return $repository;
}

protected function getCacheCollectionName(): string
private function getCacheCollectionName(): string
{
return config('cache.stores.mongodb.collection');
}

protected function withCachePrefix(string $key): string
private function withCachePrefix(string $key): string
{
return config('cache.prefix') . $key;
}

protected function insertToCacheTable(string $key, $value, $ttl = 60)
private function insertToCacheTable(string $key, $value, $ttl = 60)
{
DB::connection('mongodb')
->getCollection($this->getCacheCollectionName())
->insertOne([
'_id' => $this->withCachePrefix($key),
'value' => $value,
'expiration' => Carbon::now()->addSeconds($ttl)->getTimestamp(),
'expiration' => new UTCDateTime(Carbon::now()->addSeconds($ttl)),
]);
}
}
9 changes: 9 additions & 0 deletions tests/Cache/MongoLockTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,15 @@ public function testRestoreLock()
$this->assertFalse($resoredLock->isOwnedByCurrentProcess());
}

public function testTTLIndex()
{
$store = $this->getCache()->lock('')->createTTLIndex();

// TTL index remove expired items asynchronously, this test would be very slow
$indexes = DB::connection('mongodb')->getCollection('foo_cache_locks')->listIndexes();
$this->assertCount(2, $indexes);
}

private function getCache(): Repository
{
$repository = Cache::driver('mongodb');
Expand Down
Loading