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 all 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
56 changes: 35 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,32 +3,38 @@
namespace MongoDB\Laravel\Cache;

use Illuminate\Cache\Lock;
use Illuminate\Support\Carbon;
use InvalidArgumentException;
use MongoDB\BSON\UTCDateTime;
use MongoDB\Laravel\Collection;
use MongoDB\Operation\FindOneAndUpdate;
use Override;

use function is_numeric;
use function random_int;

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.

) {
if (! is_numeric($this->lottery[0] ?? null) || ! is_numeric($this->lottery[1] ?? null) || $this->lottery[0] > $this->lottery[1]) {
throw new InvalidArgumentException('Lock lottery must be a couple of integers [$chance, $total] where $chance <= $total. Example [2, 100]');
}

parent::__construct($name, $seconds, $owner);
}

Expand All @@ -41,7 +47,7 @@ public function acquire(): bool
// or it is already owned by the same lock instance.
$isExpiredOrAlreadyOwned = [
'$or' => [
['$lte' => ['$expiration', $this->currentTime()]],
['$lte' => ['$expires_at', $this->getUTCDateTime()]],
['$eq' => ['$owner', $this->owner]],
],
];
Expand All @@ -57,11 +63,11 @@ public function acquire(): bool
'else' => '$owner',
],
],
'expiration' => [
'expires_at' => [
'$cond' => [
'if' => $isExpiredOrAlreadyOwned,
'then' => $this->expiresAt(),
'else' => '$expiration',
'then' => $this->getUTCDateTime($this->seconds),
'else' => '$expires_at',
],
],
],
Expand All @@ -74,10 +80,12 @@ public function acquire(): bool
],
);

if (random_int(1, $this->lottery[1]) <= $this->lottery[0]) {
$this->collection->deleteMany(['expiration' => ['$lte' => $this->currentTime()]]);
if ($this->lottery[0] <= 0 && random_int(1, $this->lottery[1]) <= $this->lottery[0]) {
$this->collection->deleteMany(['expires_at' => ['$lte' => $this->getUTCDateTime()]]);
}

// Compare the owner to check if the lock is owned. Acquiring the same lock
// with the same owner at the same instant would lead to not update the document
return $result['owner'] === $this->owner;
}

Expand Down Expand Up @@ -107,6 +115,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
['expires_at' => 1],
// Delay to remove items after expiration
['expireAfterSeconds' => 0],
);
}

/**
* Returns the owner value written into the driver for this lock.
*/
Expand All @@ -116,19 +135,14 @@ protected function getCurrentOwner(): ?string
return $this->collection->findOne(
[
'_id' => $this->name,
'expiration' => ['$gte' => $this->currentTime()],
'expires_at' => ['$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!

}
}
50 changes: 32 additions & 18 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,
'expires_at' => $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' => ['$expires_at', $this->getUTCDateTime()]];

$result = $this->collection->updateOne(
[
'_id' => $this->prefix . $key,
Expand All @@ -125,16 +126,16 @@ public function add($key, $value, $seconds): bool
'$set' => [
'value' => [
'$cond' => [
'if' => ['$lte' => ['$expiration', $this->currentTime()]],
'if' => $isExpired,
'then' => $this->serialize($value),
'else' => '$value',
],
],
'expiration' => [
'expires_at' => [
'$cond' => [
'if' => ['$lte' => ['$expiration', $this->currentTime()]],
'then' => $this->currentTime() + $seconds,
'else' => '$expiration',
'if' => $isExpired,
'then' => $this->getUTCDateTime($seconds),
'else' => '$expires_at',
],
],
],
Expand All @@ -156,14 +157,14 @@ public function get($key): mixed
{
$result = $this->collection->findOne(
['_id' => $this->prefix . $key],
['projection' => ['value' => 1, 'expiration' => 1]],
['projection' => ['value' => 1, 'expires_at' => 1]],
);

if (! $result) {
return null;
}

if ($result['expiration'] <= $this->currentTime()) {
if ($result['expires_at'] <= $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['expires_at'] <= $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()],
'expires_at' => ['$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
['expires_at' => 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(),
'expires_at' => new UTCDateTime(Carbon::now()->addSeconds($ttl)),
]);
}
}
Loading
Loading