-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
PHPORM-99 Add optimized cache and lock drivers #2877
Conversation
46747a9
to
55c5327
Compare
src/Cache/MongoLock.php
Outdated
$this->collection->deleteMany(['expiration' => ['$lte' => $this->currentTime()]]); | ||
} | ||
|
||
return $result->owner === $this->owner; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I'm using findOneAndUpdate
and a comparison of the value to return true
when the lock is acquired twice by the same instance during the same second. If I use updateOne
and check $result->getUpsertedCount() > 0 || $result->getModifiedCount() > 0;
, the result is false
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when the lock is acquired twice by the same instance during the same second
What is the significance of "same instance during the same second"?
If updateOne
reports that no document was upserted or modified, wouldn't that imply that the lock could not be acquired (i.e. has another owner and has not expired)?
I was testing this locally:
$coll = $client->selectCollection('test', 'coll');
$coll->drop();
$key = 'foo';
$owner = 'bar';
$currentTime = time();
$expiresAt = $currentTime + 10;
$isExpiredOrAlreadyOwned = ['$or' => [
['$lte' => ['$expiration', $currentTime]],
['$eq' => ['$owner', $owner]],
]];
$result = $coll->updateOne(
['key' => $key],
[
['$set' => [
'owner' => ['$cond' => [$isExpiredOrAlreadyOwned, $owner, '$owner']],
'expiration' => ['$cond' => [$isExpiredOrAlreadyOwned, $expiresAt, '$expiration']],
]],
],
['upsert' => true],
);
printf("\n\nupdateOne: matched(%d) modified(%d) upserted(%d)\n\n",
$result->getMatchedCount(),
$result->getModifiedCount(),
$result->getUpsertedCount(),
);
if ($result->getMatchedCount()) {
echo "lock '$key' already existed\n";
}
if ($result->getModifiedCount()) {
echo "took over an expired lock for '$owner', which will now expire at $expiresAt\n";
}
if ($result->getUpsertedCount()) {
echo "created the lock for '$owner', which will now expire at $expiresAt\n";
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm talking about this test:
laravel-mongodb/tests/Cache/MongoLockTest.php
Lines 30 to 34 in 3fa18f3
public function testLockCanBeAcquired() | |
{ | |
$lock = $this->getCache()->lock('foo'); | |
$this->assertTrue($lock->get()); | |
$this->assertTrue($lock->get()); |
updateOne
reports no document modified if the values in $set
are not different from the values already stored.
In your example, if you run updateOne
2 times, the 2nd time you get "lock 'foo' already existed" and nothing else. In Laravel, true
is expected because the lock is acquired and owned by the current instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I understand the significance of "same second" in your first comment. The timestamp field wouldn't change on subsequent updates within the same clock second, and you rely on checking the owner
field (not possible with updateOne
) to determine if the lock is acquired.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted to updateOne
in this additional PR, because UTCDateTime
is more precise and acquiring a lock with the same owner in the same microsecond will not happen.
src/Query/Builder.php
Outdated
// Ignore "duplicate key error" | ||
if ($exception->getCode() !== 11000) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I should ignore only "duplicate key errors". Laravel docs
The
insertOrIgnore
method will ignore errors while inserting records into the database. When using this method, you should be aware that duplicate record errors will be ignored and other types of errors may also be ignored depending on the database engine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dislike the ambiguity that is built into the documentation - you can't really be sure what's ignored. One option is to ignore any errors that purely come from the data (e.g. unique key violations, document constraint violations) but still report more serious problems (like server selection errors). IMO, users should be more aware of what could go wrong and explicitly ignore those errors if they don't care for them.
For the purpose of the cache/lock work, this seems sufficient so I'm fine with only ignoring unique key errors and returning to this if people have more errors they think should be ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is no longer used by the cache. I'm reverting this change, and creating a dedicated ticket PHPORM-170
// Provides "many" and "putMany" in a non-optimized way | ||
use RetrievesMultipleKeys; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be improved later in a new PR.
@@ -25,13 +25,15 @@ | |||
"php": "^8.1", | |||
"ext-mongodb": "^1.15", | |||
"composer-runtime-api": "^2.0.0", | |||
"illuminate/support": "^10.0|^11", | |||
"illuminate/cache": "^10.36|^11", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Required to access Cache::isOwnedCurrentProcess
laravel/framework@234444e
], | ||
); | ||
|
||
if (random_int(1, $this->lottery[1]) <= $this->lottery[0]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize this is beyond the PR, but is there a particular reason that [2, 100]
is used instead of 0.02
? This just seems like an odd API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comes from Laravel's Lottery: https://laravel.com/api/11.x/Illuminate/Support/Lottery.html
Used in DatabaseLock config: https://github.com/laravel/framework/blob/9b3bb15ab4d8e7a80bda82ffbe92613f15f69fee/src/Illuminate/Cache/DatabaseLock.php#L49
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see see the same structure used in DatabaseLock, but not the Lottery class itself. Consistency is probably more important here. It may be worth elaborating on the structure of that two-element array in the doc block comment, though. As is, it's not entirely clear how the two values relate to one another.
src/Cache/MongoLock.php
Outdated
$this->collection->deleteMany(['expiration' => ['$lte' => $this->currentTime()]]); | ||
} | ||
|
||
return $result->owner === $this->owner; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when the lock is acquired twice by the same instance during the same second
What is the significance of "same instance during the same second"?
If updateOne
reports that no document was upserted or modified, wouldn't that imply that the lock could not be acquired (i.e. has another owner and has not expired)?
I was testing this locally:
$coll = $client->selectCollection('test', 'coll');
$coll->drop();
$key = 'foo';
$owner = 'bar';
$currentTime = time();
$expiresAt = $currentTime + 10;
$isExpiredOrAlreadyOwned = ['$or' => [
['$lte' => ['$expiration', $currentTime]],
['$eq' => ['$owner', $owner]],
]];
$result = $coll->updateOne(
['key' => $key],
[
['$set' => [
'owner' => ['$cond' => [$isExpiredOrAlreadyOwned, $owner, '$owner']],
'expiration' => ['$cond' => [$isExpiredOrAlreadyOwned, $expiresAt, '$expiration']],
]],
],
['upsert' => true],
);
printf("\n\nupdateOne: matched(%d) modified(%d) upserted(%d)\n\n",
$result->getMatchedCount(),
$result->getModifiedCount(),
$result->getUpsertedCount(),
);
if ($result->getMatchedCount()) {
echo "lock '$key' already existed\n";
}
if ($result->getModifiedCount()) {
echo "took over an expired lock for '$owner', which will now expire at $expiresAt\n";
}
if ($result->getUpsertedCount()) {
echo "created the lock for '$owner', which will now expire at $expiresAt\n";
}
src/Cache/MongoLock.php
Outdated
$this->collection->deleteMany(['expiration' => ['$lte' => $this->currentTime()]]); | ||
} | ||
|
||
return $result->owner === $this->owner; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I understand the significance of "same second" in your first comment. The timestamp field wouldn't change on subsequent updates within the same clock second, and you rely on checking the owner
field (not possible with updateOne
) to determine if the lock is acquired.
|
||
public function flush(): bool | ||
{ | ||
$this->collection->deleteMany([]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you consider dropping the collection? My storage engine knowledge is outdated, but I still reckon that'd be a faster operation.
And if there's no concern about preserving indexes or other metadata (e.g. validation rules), there isn't much benefit to keeping the original collection in place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not, if the engine supports it well. What is the risk of race-conditioning if there is an insert at the same time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropping locks the collection, and thus I expect it'd avoid any potential race conditions. In contrast, I expect deleteMany()
would allow other operations to interleave.
To clarify: I don't consider this a real risk for the PR. The drop just seemed like a more straightforward operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to drop the collection. You could legitimately want to create a TTL index on the expiration
field, and dropping the collection to purge the cache would have the side-effect of deleting the index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See additional PR that enable TTL index: #2891
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've said my peace, so I'll defer to you to incorporate any feedback you like.
3df8f45
to
3f42383
Compare
$this->collection->deleteMany(['expiration' => ['$lte' => $this->currentTime()]]); | ||
} | ||
|
||
return $result['owner'] === $this->owner; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted to use array-access to please static analysis tools that doesn't understand that MongoDB\MongoDB\BSONDocument
has magic property-access.
https://phpstan.org/r/1e7b5d92-8b98-4e53-863f-b0c8652820c1
https://psalm.dev/r/ab76084ecf
$doc = new ArrayObject(['foo' => 'bar'], ArrayObject::ARRAY_AS_PROPS);
$doc->bar;
It's also more robust in case array
is used as default typemap.
|
||
public function flush(): bool | ||
{ | ||
$this->collection->deleteMany([]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to drop the collection. You could legitimately want to create a TTL index on the expiration
field, and dropping the collection to purge the cache would have the side-effect of deleting the index.
Fix fo PHPORM-99
In theory, we can use
DatabaseStore
andDatabaseLock
. But various issues prove the changing nature of their implementation, based on new features in the query builder, make this feature unstable for MongoDB users. fix #2718, fix #2609By introducing dedicated drivers, we can optimize the implementation to use the mongodb library directly instead of the subset of features provided by laravel query builder.
Usage:
Cache:
Lock:
Checklist