Skip to content

Commit ba55d7e

Browse files
AndyScherzingerAndyScherzinger
authored
Merge pull request #54967 from nextcloud/backport/52825/stable32
[stable32] feat(files_trashbin): Refactor expire background job to support parallel run
2 parents 4ecdda4 + a90299e commit ba55d7e

5 files changed

Lines changed: 126 additions & 64 deletions

File tree

‎apps/files_trashbin/lib/BackgroundJob/ExpireTrash.php‎

Lines changed: 79 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -7,32 +7,42 @@
77
*/
88
namespace OCA\Files_Trashbin\BackgroundJob;
99

10+
use OC\Files\SetupManager;
1011
use OC\Files\View;
12+
use OCA\Files_Trashbin\AppInfo\Application;
1113
use OCA\Files_Trashbin\Expiration;
1214
use OCA\Files_Trashbin\Helper;
1315
use OCA\Files_Trashbin\Trashbin;
1416
use OCP\AppFramework\Utility\ITimeFactory;
1517
use OCP\BackgroundJob\TimedJob;
1618
use OCP\IAppConfig;
19+
use OCP\IUser;
1720
use OCP\IUserManager;
21+
use OCP\Lock\ILockingProvider;
1822
use Psr\Log\LoggerInterface;
1923

2024
class ExpireTrash extends TimedJob {
25+
public const TOGGLE_CONFIG_KEY_NAME = 'background_job_expire_trash';
26+
public const OFFSET_CONFIG_KEY_NAME = 'background_job_expire_trash_offset';
27+
private const THIRTY_MINUTES = 30 * 60;
28+
private const USER_BATCH_SIZE = 10;
29+
2130
public function __construct(
2231
private IAppConfig $appConfig,
2332
private IUserManager $userManager,
2433
private Expiration $expiration,
2534
private LoggerInterface $logger,
35+
private SetupManager $setupManager,
36+
private ILockingProvider $lockingProvider,
2637
ITimeFactory $time,
2738
) {
2839
parent::__construct($time);
29-
// Run once per 30 minutes
30-
$this->setInterval(60 * 30);
40+
$this->setInterval(self::THIRTY_MINUTES);
3141
}
3242

3343
protected function run($argument) {
34-
$backgroundJob = $this->appConfig->getValueString('files_trashbin', 'background_job_expire_trash', 'yes');
35-
if ($backgroundJob === 'no') {
44+
$backgroundJob = $this->appConfig->getValueBool(Application::APP_ID, self::TOGGLE_CONFIG_KEY_NAME, true);
45+
if (!$backgroundJob) {
3646
return;
3747
}
3848

@@ -41,48 +51,89 @@ protected function run($argument) {
4151
return;
4252
}
4353

44-
$stopTime = time() + 60 * 30; // Stops after 30 minutes.
45-
$offset = $this->appConfig->getValueInt('files_trashbin', 'background_job_expire_trash_offset', 0);
46-
$users = $this->userManager->getSeenUsers($offset);
54+
$startTime = time();
4755

48-
foreach ($users as $user) {
49-
try {
56+
// Process users in batches of 10, but don't run for more than 30 minutes
57+
while (time() < $startTime + self::THIRTY_MINUTES) {
58+
$offset = $this->getNextOffset();
59+
$users = $this->userManager->getSeenUsers($offset, self::USER_BATCH_SIZE);
60+
$count = 0;
61+
62+
foreach ($users as $user) {
5063
$uid = $user->getUID();
51-
if (!$this->setupFS($uid)) {
52-
continue;
64+
$count++;
65+
66+
try {
67+
if ($this->setupFS($user)) {
68+
$dirContent = Helper::getTrashFiles('/', $uid, 'mtime');
69+
Trashbin::deleteExpiredFiles($dirContent, $uid);
70+
}
71+
} catch (\Throwable $e) {
72+
$this->logger->error('Error while expiring trashbin for user ' . $uid, ['exception' => $e]);
73+
} finally {
74+
$this->setupManager->tearDown();
5375
}
54-
$dirContent = Helper::getTrashFiles('/', $uid, 'mtime');
55-
Trashbin::deleteExpiredFiles($dirContent, $uid);
56-
} catch (\Throwable $e) {
57-
$this->logger->error('Error while expiring trashbin for user ' . $user->getUID(), ['exception' => $e]);
5876
}
5977

60-
$offset++;
61-
62-
if ($stopTime < time()) {
63-
$this->appConfig->setValueInt('files_trashbin', 'background_job_expire_trash_offset', $offset);
64-
\OC_Util::tearDownFS();
65-
return;
78+
// If the last batch was not full it means that we reached the end of the user list.
79+
if ($count < self::USER_BATCH_SIZE) {
80+
$this->resetOffset();
6681
}
6782
}
68-
69-
$this->appConfig->setValueInt('files_trashbin', 'background_job_expire_trash_offset', 0);
70-
\OC_Util::tearDownFS();
7183
}
7284

7385
/**
7486
* Act on behalf on trash item owner
7587
*/
76-
protected function setupFS(string $user): bool {
77-
\OC_Util::tearDownFS();
78-
\OC_Util::setupFS($user);
88+
protected function setupFS(IUser $user): bool {
89+
$this->setupManager->setupForUser($user);
7990

8091
// Check if this user has a trashbin directory
81-
$view = new View('/' . $user);
92+
$view = new View('/' . $user->getUID());
8293
if (!$view->is_dir('/files_trashbin/files')) {
8394
return false;
8495
}
8596

8697
return true;
8798
}
99+
100+
private function getNextOffset(): int {
101+
return $this->runMutexOperation(function () {
102+
$this->appConfig->clearCache();
103+
104+
$offset = $this->appConfig->getValueInt(Application::APP_ID, self::OFFSET_CONFIG_KEY_NAME, 0);
105+
$this->appConfig->setValueInt(Application::APP_ID, self::OFFSET_CONFIG_KEY_NAME, $offset + self::USER_BATCH_SIZE);
106+
107+
return $offset;
108+
});
109+
110+
}
111+
112+
private function resetOffset() {
113+
$this->runMutexOperation(function () {
114+
$this->appConfig->setValueInt(Application::APP_ID, self::OFFSET_CONFIG_KEY_NAME, 0);
115+
});
116+
}
117+
118+
private function runMutexOperation($operation): mixed {
119+
$acquired = false;
120+
121+
while ($acquired === false) {
122+
try {
123+
$this->lockingProvider->acquireLock(self::OFFSET_CONFIG_KEY_NAME, ILockingProvider::LOCK_EXCLUSIVE, 'Expire trashbin background job offset');
124+
$acquired = true;
125+
} catch (\OCP\Lock\LockedException $e) {
126+
// wait a bit and try again
127+
usleep(100000);
128+
}
129+
}
130+
131+
try {
132+
$result = $operation();
133+
} finally {
134+
$this->lockingProvider->releaseLock(self::OFFSET_CONFIG_KEY_NAME, ILockingProvider::LOCK_EXCLUSIVE);
135+
}
136+
137+
return $result;
138+
}
88139
}

‎apps/files_trashbin/tests/BackgroundJob/ExpireTrashTest.php‎

Lines changed: 32 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,15 @@
99

1010
namespace OCA\Files_Trashbin\Tests\BackgroundJob;
1111

12+
use OC\Files\SetupManager;
13+
use OCA\Files_Trashbin\AppInfo\Application;
1214
use OCA\Files_Trashbin\BackgroundJob\ExpireTrash;
1315
use OCA\Files_Trashbin\Expiration;
1416
use OCP\AppFramework\Utility\ITimeFactory;
1517
use OCP\BackgroundJob\IJobList;
1618
use OCP\IAppConfig;
1719
use OCP\IUserManager;
20+
use OCP\Lock\ILockingProvider;
1821
use PHPUnit\Framework\MockObject\MockObject;
1922
use Psr\Log\LoggerInterface;
2023
use Test\TestCase;
@@ -26,6 +29,8 @@ class ExpireTrashTest extends TestCase {
2629
private IJobList&MockObject $jobList;
2730
private LoggerInterface&MockObject $logger;
2831
private ITimeFactory&MockObject $time;
32+
private SetupManager&MockObject $setupManager;
33+
private ILockingProvider&MockObject $lockingProvider;
2934

3035
protected function setUp(): void {
3136
parent::setUp();
@@ -35,6 +40,8 @@ protected function setUp(): void {
3540
$this->expiration = $this->createMock(Expiration::class);
3641
$this->jobList = $this->createMock(IJobList::class);
3742
$this->logger = $this->createMock(LoggerInterface::class);
43+
$this->setupManager = $this->createMock(SetupManager::class);
44+
$this->lockingProvider = $this->createMock(ILockingProvider::class);
3845

3946
$this->time = $this->createMock(ITimeFactory::class);
4047
$this->time->method('getTime')
@@ -47,25 +54,41 @@ protected function setUp(): void {
4754
}
4855

4956
public function testConstructAndRun(): void {
50-
$this->appConfig->method('getValueString')
51-
->with('files_trashbin', 'background_job_expire_trash', 'yes')
52-
->willReturn('yes');
57+
$this->appConfig->method('getValueBool')
58+
->with(Application::APP_ID, ExpireTrash::TOGGLE_CONFIG_KEY_NAME, true)
59+
->willReturn(true);
5360
$this->appConfig->method('getValueInt')
54-
->with('files_trashbin', 'background_job_expire_trash_offset', 0)
61+
->with(Application::APP_ID, ExpireTrash::OFFSET_CONFIG_KEY_NAME, 0)
5562
->willReturn(0);
5663

57-
$job = new ExpireTrash($this->appConfig, $this->userManager, $this->expiration, $this->logger, $this->time);
64+
$job = new ExpireTrash(
65+
$this->appConfig,
66+
$this->userManager,
67+
$this->expiration,
68+
$this->logger,
69+
$this->setupManager,
70+
$this->lockingProvider,
71+
$this->time,
72+
);
5873
$job->start($this->jobList);
5974
}
6075

6176
public function testBackgroundJobDeactivated(): void {
62-
$this->appConfig->method('getValueString')
63-
->with('files_trashbin', 'background_job_expire_trash', 'yes')
64-
->willReturn('no');
77+
$this->appConfig->method('getValueBool')
78+
->with(Application::APP_ID, ExpireTrash::TOGGLE_CONFIG_KEY_NAME, true)
79+
->willReturn(false);
6580
$this->expiration->expects($this->never())
6681
->method('getMaxAgeAsTimestamp');
6782

68-
$job = new ExpireTrash($this->appConfig, $this->userManager, $this->expiration, $this->logger, $this->time);
83+
$job = new ExpireTrash(
84+
$this->appConfig,
85+
$this->userManager,
86+
$this->expiration,
87+
$this->logger,
88+
$this->setupManager,
89+
$this->lockingProvider,
90+
$this->time,
91+
);
6992
$job->start($this->jobList);
7093
}
7194
}

‎build/psalm-baseline.xml‎

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1763,19 +1763,6 @@
17631763
<code><![CDATA[moveMount]]></code>
17641764
</UndefinedMethod>
17651765
</file>
1766-
<file src="apps/files_trashbin/lib/BackgroundJob/ExpireTrash.php">
1767-
<DeprecatedClass>
1768-
<code><![CDATA[\OC_Util::setupFS($user)]]></code>
1769-
<code><![CDATA[\OC_Util::tearDownFS()]]></code>
1770-
<code><![CDATA[\OC_Util::tearDownFS()]]></code>
1771-
<code><![CDATA[\OC_Util::tearDownFS()]]></code>
1772-
</DeprecatedClass>
1773-
<DeprecatedMethod>
1774-
<code><![CDATA[\OC_Util::tearDownFS()]]></code>
1775-
<code><![CDATA[\OC_Util::tearDownFS()]]></code>
1776-
<code><![CDATA[\OC_Util::tearDownFS()]]></code>
1777-
</DeprecatedMethod>
1778-
</file>
17791766
<file src="apps/files_trashbin/lib/Command/CleanUp.php">
17801767
<DeprecatedClass>
17811768
<code><![CDATA[\OC_Util::setupFS($uid)]]></code>

‎lib/private/User/Manager.php‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -634,7 +634,7 @@ public function callForSeenUsers(\Closure $callback) {
634634
}
635635

636636
/**
637-
* Getting all userIds that have a listLogin value requires checking the
637+
* Getting all userIds that have a lastLogin value requires checking the
638638
* value in php because on oracle you cannot use a clob in a where clause,
639639
* preventing us from doing a not null or length(value) > 0 check.
640640
*
@@ -813,19 +813,19 @@ public function getDisplayNameCache(): DisplayNameCache {
813813
return $this->displayNameCache;
814814
}
815815

816-
/**
817-
* Gets the list of users sorted by lastLogin, from most recent to least recent
818-
*
819-
* @param int $offset from which offset to fetch
820-
* @return \Iterator<IUser> list of user IDs
821-
* @since 30.0.0
822-
*/
823-
public function getSeenUsers(int $offset = 0): \Iterator {
824-
$limit = 1000;
816+
public function getSeenUsers(int $offset = 0, ?int $limit = null): \Iterator {
817+
$maxBatchSize = 1000;
825818

826819
do {
827-
$userIds = $this->getSeenUserIds($limit, $offset);
828-
$offset += $limit;
820+
if ($limit !== null) {
821+
$batchSize = min($limit, $maxBatchSize);
822+
$limit -= $batchSize;
823+
} else {
824+
$batchSize = $maxBatchSize;
825+
}
826+
827+
$userIds = $this->getSeenUserIds($batchSize, $offset);
828+
$offset += $batchSize;
829829

830830
foreach ($userIds as $userId) {
831831
foreach ($this->backends as $backend) {
@@ -836,6 +836,6 @@ public function getSeenUsers(int $offset = 0): \Iterator {
836836
}
837837
}
838838
}
839-
} while (count($userIds) === $limit);
839+
} while (count($userIds) === $batchSize && $limit !== 0);
840840
}
841841
}

‎lib/public/IUserManager.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -239,8 +239,9 @@ public function getLastLoggedInUsers(?int $limit = null, int $offset = 0, string
239239
* The offset argument allows the caller to continue the iteration at a specific offset.
240240
*
241241
* @param int $offset from which offset to fetch
242+
* @param int|null $limit maximum number of records to fetch
242243
* @return \Iterator<IUser> list of IUser object
243244
* @since 32.0.0
244245
*/
245-
public function getSeenUsers(int $offset = 0): \Iterator;
246+
public function getSeenUsers(int $offset = 0, ?int $limit = null): \Iterator;
246247
}

0 commit comments

Comments
 (0)