Skip to content

Commit dddf105

Browse files
artongeartonge
committed
feat(files_trashbin): Refactor expire background job to support parallel run
- Follow-up of #51600 The original PR introduced the possibility to continue an `ExpireTrash` job by saving the offset. This was to prevent having to start over the whole user list when the job crashed or was killed. But on big instances, one process is not enough to go through all the users in a timely manner. Supporting parallel run allows covering more ground faster. This PR introduced this possibility. We are now storing the offset right away to allow another parallel job to pick up the task at that point. We are arbitrarily cutting the user list in chunk of 10 to not drastically overflow the 30 minutes time limit. Signed-off-by: Louis Chemineau <louis@chmn.me>
1 parent 0313ab3 commit dddf105

File tree

2 files changed

+113
-38
lines changed

2 files changed

+113
-38
lines changed

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

Lines changed: 80 additions & 29 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;
11+
use OC\Files\View;
12+
use OCA\Files_Trashbin\AppInfo\Application;
1013
use OCA\Files_Trashbin\Expiration;
1114
use OCA\Files_Trashbin\Helper;
1215
use OCA\Files_Trashbin\Trashbin;
1316
use OCP\AppFramework\Utility\ITimeFactory;
1417
use OCP\BackgroundJob\TimedJob;
1518
use OCP\IAppConfig;
19+
use OCP\IUser;
1620
use OCP\IUserManager;
21+
use OCP\Lock\ILockingProvider;
1722
use Psr\Log\LoggerInterface;
1823

1924
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;
2029

2130
public function __construct(
2231
private IAppConfig $appConfig,
2332
private IUserManager $userManager,
2433
private Expiration $expiration,
2534
private LoggerInterface $logger,
26-
ITimeFactory $time
35+
private SetupManager $setupManager,
36+
private ILockingProvider $lockingProvider,
37+
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 \OC\Files\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: 33 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,15 @@
88

99
namespace OCA\Files_Trashbin\Tests\BackgroundJob;
1010

11+
use OC\Files\SetupManager;
12+
use OCA\Files_Trashbin\AppInfo\Application;
1113
use OCA\Files_Trashbin\BackgroundJob\ExpireTrash;
1214
use OCA\Files_Trashbin\Expiration;
1315
use OCP\AppFramework\Utility\ITimeFactory;
1416
use OCP\BackgroundJob\IJobList;
1517
use OCP\IAppConfig;
1618
use OCP\IUserManager;
19+
use OCP\Lock\ILockingProvider;
1720
use PHPUnit\Framework\MockObject\MockObject;
1821
use Psr\Log\LoggerInterface;
1922
use Test\TestCase;
@@ -37,6 +40,9 @@ class ExpireTrashTest extends TestCase {
3740
/** @var ITimeFactory&MockObject */
3841
private $time;
3942

43+
private SetupManager&MockObject $setupManager;
44+
private ILockingProvider&MockObject $lockingProvider;
45+
4046
protected function setUp(): void {
4147
parent::setUp();
4248

@@ -45,6 +51,8 @@ protected function setUp(): void {
4551
$this->expiration = $this->createMock(Expiration::class);
4652
$this->jobList = $this->createMock(IJobList::class);
4753
$this->logger = $this->createMock(LoggerInterface::class);
54+
$this->setupManager = $this->createMock(SetupManager::class);
55+
$this->lockingProvider = $this->createMock(ILockingProvider::class);
4856

4957
$this->time = $this->createMock(ITimeFactory::class);
5058
$this->time->method('getTime')
@@ -57,25 +65,41 @@ protected function setUp(): void {
5765
}
5866

5967
public function testConstructAndRun(): void {
60-
$this->appConfig->method('getValueString')
61-
->with('files_trashbin', 'background_job_expire_trash', 'yes')
62-
->willReturn('yes');
68+
$this->appConfig->method('getValueBool')
69+
->with(Application::APP_ID, ExpireTrash::TOGGLE_CONFIG_KEY_NAME, true)
70+
->willReturn(true);
6371
$this->appConfig->method('getValueInt')
64-
->with('files_trashbin', 'background_job_expire_trash_offset', 0)
72+
->with(Application::APP_ID, ExpireTrash::OFFSET_CONFIG_KEY_NAME, 0)
6573
->willReturn(0);
6674

67-
$job = new ExpireTrash($this->appConfig, $this->userManager, $this->expiration, $this->logger, $this->time);
75+
$job = new ExpireTrash(
76+
$this->appConfig,
77+
$this->userManager,
78+
$this->expiration,
79+
$this->logger,
80+
$this->setupManager,
81+
$this->lockingProvider,
82+
$this->time,
83+
);
6884
$job->start($this->jobList);
6985
}
7086

7187
public function testBackgroundJobDeactivated(): void {
72-
$this->appConfig->method('getValueString')
73-
->with('files_trashbin', 'background_job_expire_trash', 'yes')
74-
->willReturn('no');
88+
$this->appConfig->method('getValueBool')
89+
->with(Application::APP_ID, ExpireTrash::TOGGLE_CONFIG_KEY_NAME, true)
90+
->willReturn(false);
7591
$this->expiration->expects($this->never())
7692
->method('getMaxAgeAsTimestamp');
7793

78-
$job = new ExpireTrash($this->appConfig, $this->userManager, $this->expiration, $this->logger, $this->time);
94+
$job = new ExpireTrash(
95+
$this->appConfig,
96+
$this->userManager,
97+
$this->expiration,
98+
$this->logger,
99+
$this->setupManager,
100+
$this->lockingProvider,
101+
$this->time,
102+
);
79103
$job->start($this->jobList);
80104
}
81105
}

0 commit comments

Comments
 (0)