Skip to content

Commit f6d18f8

Browse files
icewind1991icewind1991
committed
perf: merge last_activity and last_check updates
the debounce for updating last_activity is changed so it always updates if another field of the token has been updated, this ensures that last_check if updated even if last_activity is still in the debounce period. Signed-off-by: Robin Appelman <robin@icewind.nl>
1 parent f56bca3 commit f6d18f8

6 files changed

Lines changed: 113 additions & 29 deletions

File tree

‎lib/private/Authentication/Token/PublicKeyTokenMapper.php‎

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,7 @@ public function hasExpiredTokens(string $uid): bool {
182182
}
183183

184184
/**
185-
* Update the last activity timestamp
185+
* Update the last activity timestamp and save all saved fields
186186
*
187187
* In highly concurrent setups it can happen that two parallel processes
188188
* trigger the update at (nearly) the same time. In that special case it's
@@ -192,7 +192,7 @@ public function hasExpiredTokens(string $uid): bool {
192192
*
193193
* Example:
194194
* - process 1 (P1) reads the token at timestamp 1500
195-
* - process 1 (P2) reads the token at timestamp 1501
195+
* - process 2 (P2) reads the token at timestamp 1501
196196
* - activity update interval is 100
197197
*
198198
* This means
@@ -206,17 +206,21 @@ public function hasExpiredTokens(string $uid): bool {
206206
* but the comparison on last_activity will still not be truthy and the
207207
* token row is not updated a second time
208208
*
209-
* @param IToken $token
209+
* @param PublicKeyToken $token
210210
* @param int $now
211211
*/
212-
public function updateActivity(IToken $token, int $now): void {
213-
$qb = $this->db->getQueryBuilder();
214-
$update = $qb->update($this->getTableName())
215-
->set('last_activity', $qb->createNamedParameter($now, IQueryBuilder::PARAM_INT))
216-
->where(
217-
$qb->expr()->eq('id', $qb->createNamedParameter($token->getId(), IQueryBuilder::PARAM_INT), IQueryBuilder::PARAM_INT),
218-
$qb->expr()->lt('last_activity', $qb->createNamedParameter($now - 15, IQueryBuilder::PARAM_INT), IQueryBuilder::PARAM_INT)
219-
);
212+
public function updateActivity(PublicKeyToken $token, int $now): void {
213+
$token->setLastActivity($now);
214+
$update = $this->createUpdateQuery($token);
215+
216+
$updatedFields = $token->getUpdatedFields();
217+
unset($updatedFields['lastActivity']);
218+
219+
// if no other fields are updated, we add the extra filter to prevent duplicate updates
220+
if (count($updatedFields) === 0) {
221+
$update->andWhere($update->expr()->lt('last_activity', $update->createNamedParameter($now - 15, IQueryBuilder::PARAM_INT), IQueryBuilder::PARAM_INT));
222+
}
223+
220224
$update->executeStatement();
221225
}
222226

‎lib/private/Authentication/Token/PublicKeyTokenProvider.php‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -299,10 +299,12 @@ public function updateTokenActivity(OCPIToken $token) {
299299
$activityInterval = $this->config->getSystemValueInt('token_auth_activity_update', 60);
300300
$activityInterval = min(max($activityInterval, 0), 300);
301301

302+
$updatedFields = $token->getUpdatedFields();
303+
unset($updatedFields['lastActivity']);
304+
302305
/** @var PublicKeyToken $token */
303306
$now = $this->time->getTime();
304-
if ($token->getLastActivity() < ($now - $activityInterval)) {
305-
$token->setLastActivity($now);
307+
if ($token->getLastActivity() < ($now - $activityInterval) || count($updatedFields)) {
306308
$this->mapper->updateActivity($token, $now);
307309
$this->cacheToken($token);
308310
}

‎lib/private/User/Session.php‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -757,7 +757,6 @@ private function checkTokenCredentials(IToken $dbToken, $token) {
757757
if ($dbToken instanceof PublicKeyToken) {
758758
$dbToken->setLastActivity($now);
759759
}
760-
$this->tokenProvider->updateToken($dbToken);
761760
return true;
762761
}
763762

‎lib/public/AppFramework/Db/QBMapper.php‎

Lines changed: 29 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -146,22 +146,15 @@ public function insertOrUpdate(Entity $entity): Entity {
146146
}
147147

148148
/**
149-
* Updates an entry in the db from an entity
149+
* Create an update query that saves all updated fields
150150
*
151-
* @param Entity $entity the entity that should be created
152-
* @psalm-param T $entity the entity that should be created
153-
* @return Entity the saved entity with the set id
154-
* @psalm-return T the saved entity with the set id
155-
* @throws Exception
156-
* @throws \InvalidArgumentException if entity has no id
157-
* @since 14.0.0
151+
* @param Entity $entity the entity that should be updated
152+
* @psalm-param T $entity the entity that should be updated
153+
* @return IQueryBuilder
154+
* @since 25.0.0
158155
*/
159-
public function update(Entity $entity): Entity {
160-
// if entity wasn't changed it makes no sense to run a db query
156+
protected function createUpdateQuery(Entity $entity): IQueryBuilder {
161157
$properties = $entity->getUpdatedFields();
162-
if (\count($properties) === 0) {
163-
return $entity;
164-
}
165158

166159
// entity needs an id
167160
$id = $entity->getId();
@@ -193,6 +186,29 @@ public function update(Entity $entity): Entity {
193186
$qb->where(
194187
$qb->expr()->eq('id', $qb->createNamedParameter($id, $idType))
195188
);
189+
190+
return $qb;
191+
}
192+
193+
/**
194+
* Updates an entry in the db from an entity
195+
*
196+
* @param Entity $entity the entity that should be created
197+
* @psalm-param T $entity the entity that should be created
198+
* @return Entity the saved entity with the set id
199+
* @psalm-return T the saved entity with the set id
200+
* @throws Exception
201+
* @throws \InvalidArgumentException if entity has no id
202+
* @since 14.0.0
203+
*/
204+
public function update(Entity $entity): Entity {
205+
// if entity wasn't changed it makes no sense to run a db query
206+
$properties = $entity->getUpdatedFields();
207+
if (\count($properties) === 0) {
208+
return $entity;
209+
}
210+
211+
$qb = $this->createUpdateQuery($entity);
196212
$qb->executeStatement();
197213

198214
return $entity;

‎tests/lib/Authentication/Token/PublicKeyTokenMapperTest.php‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -261,4 +261,53 @@ public function testHasExpiredTokens() {
261261
$this->assertFalse($this->mapper->hasExpiredTokens('user1'));
262262
$this->assertTrue($this->mapper->hasExpiredTokens('user3'));
263263
}
264+
265+
public function testUpdateTokenActivity() {
266+
$token = '6d9a290d239d09f2cc33a03cc54cccd46f7dc71630dcc27d39214824bd3e093f1feb4e2b55eb159d204caa15dee9556c202a5aa0b9d67806c3f4ec2cde11af67';
267+
$dbToken = $this->mapper->getToken($token);
268+
269+
$this->assertEquals($dbToken->getLastActivity(), $this->time - 120);
270+
$this->assertEquals($this->time - 60 * 10, $dbToken->getLastCheck());
271+
272+
$this->mapper->updateActivity($dbToken, $this->time);
273+
274+
$updatedDbToken = $this->mapper->getToken($token);
275+
276+
$this->assertEquals($this->time, $updatedDbToken->getLastActivity());
277+
$this->assertEquals($this->time - 60 * 10, $dbToken->getLastCheck());
278+
$this->assertEquals($this->time, $dbToken->getLastActivity());
279+
}
280+
281+
public function testUpdateTokenActivityDebounce() {
282+
$token = '6d9a290d239d09f2cc33a03cc54cccd46f7dc71630dcc27d39214824bd3e093f1feb4e2b55eb159d204caa15dee9556c202a5aa0b9d67806c3f4ec2cde11af67';
283+
$dbToken = $this->mapper->getToken($token);
284+
285+
$this->assertEquals($dbToken->getLastActivity(), $this->time - 120);
286+
$this->assertEquals($this->time - 60 * 10, $dbToken->getLastCheck());
287+
288+
$this->mapper->updateActivity($dbToken, $this->time - 110);
289+
290+
$updatedDbToken = $this->mapper->getToken($token);
291+
292+
$this->assertEquals($this->time - 120, $updatedDbToken->getLastActivity());
293+
$this->assertEquals($this->time - 60 * 10, $dbToken->getLastCheck());
294+
$this->assertEquals($this->time - 110, $dbToken->getLastActivity());
295+
}
296+
297+
public function testUpdateTokenActivityDebounceUpdate() {
298+
$token = '6d9a290d239d09f2cc33a03cc54cccd46f7dc71630dcc27d39214824bd3e093f1feb4e2b55eb159d204caa15dee9556c202a5aa0b9d67806c3f4ec2cde11af67';
299+
$dbToken = $this->mapper->getToken($token);
300+
301+
$this->assertEquals($this->time - 120, $dbToken->getLastActivity());
302+
$this->assertEquals($this->time - 60 * 10, $dbToken->getLastCheck());
303+
304+
$dbToken->setLastCheck($this->time - 100);
305+
$this->mapper->updateActivity($dbToken, $this->time - 110);
306+
307+
$updatedDbToken = $this->mapper->getToken($token);
308+
309+
$this->assertEquals($this->time - 110, $updatedDbToken->getLastActivity());
310+
$this->assertEquals($this->time - 100, $dbToken->getLastCheck());
311+
$this->assertEquals($this->time - 110, $dbToken->getLastActivity());
312+
}
264313
}

‎tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -191,8 +191,6 @@ public function testUpdateToken() {
191191
]);
192192

193193
$this->tokenProvider->updateTokenActivity($tk);
194-
195-
$this->assertEquals($this->time, $tk->getLastActivity());
196194
}
197195

198196
public function testUpdateTokenDebounce() {
@@ -210,6 +208,22 @@ public function testUpdateTokenDebounce() {
210208
$this->tokenProvider->updateTokenActivity($tk);
211209
}
212210

211+
public function testUpdateTokenDebounceWithUpdatedFields() {
212+
$tk = new PublicKeyToken();
213+
$this->config->method('getSystemValueInt')
214+
->willReturnCallback(function ($value, $default) {
215+
return $default;
216+
});
217+
$tk->setLastActivity($this->time - 30);
218+
$tk->setLastCheck($this->time - 30);
219+
220+
$this->mapper->expects($this->once())
221+
->method('updateActivity')
222+
->with($tk, $this->time);
223+
224+
$this->tokenProvider->updateTokenActivity($tk);
225+
}
226+
213227
public function testGetTokenByUser() {
214228
$this->mapper->expects($this->once())
215229
->method('getTokenByUser')

0 commit comments

Comments
 (0)