Skip to content

Commit a9daf61

Browse files
artongeartonge
authored
Merge pull request #53026 from nextcloud/backport/52825/stable31
2 parents 96c5996 + 60455d7 commit a9daf61

File tree

4 files changed

+127
-51
lines changed

4 files changed

+127
-51
lines changed

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

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

9+
use OC\Files\SetupManager;
910
use OC\Files\View;
11+
use OCA\Files_Trashbin\AppInfo\Application;
1012
use OCA\Files_Trashbin\Expiration;
1113
use OCA\Files_Trashbin\Helper;
1214
use OCA\Files_Trashbin\Trashbin;
1315
use OCP\AppFramework\Utility\ITimeFactory;
1416
use OCP\BackgroundJob\TimedJob;
1517
use OCP\IAppConfig;
18+
use OCP\IUser;
1619
use OCP\IUserManager;
20+
use OCP\Lock\ILockingProvider;
1721
use Psr\Log\LoggerInterface;
1822

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

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

@@ -40,48 +50,89 @@ protected function run($argument) {
4050
return;
4151
}
4252

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

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

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

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

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

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

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

Lines changed: 33 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,15 @@
77

88
namespace OCA\Files_Trashbin\Tests\BackgroundJob;
99

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

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

@@ -44,6 +50,8 @@ protected function setUp(): void {
4450
$this->expiration = $this->createMock(Expiration::class);
4551
$this->jobList = $this->createMock(IJobList::class);
4652
$this->logger = $this->createMock(LoggerInterface::class);
53+
$this->setupManager = $this->createMock(SetupManager::class);
54+
$this->lockingProvider = $this->createMock(ILockingProvider::class);
4755

4856
$this->time = $this->createMock(ITimeFactory::class);
4957
$this->time->method('getTime')
@@ -56,25 +64,41 @@ protected function setUp(): void {
5664
}
5765

5866
public function testConstructAndRun(): void {
59-
$this->appConfig->method('getValueString')
60-
->with('files_trashbin', 'background_job_expire_trash', 'yes')
61-
->willReturn('yes');
67+
$this->appConfig->method('getValueBool')
68+
->with(Application::APP_ID, ExpireTrash::TOGGLE_CONFIG_KEY_NAME, true)
69+
->willReturn(true);
6270
$this->appConfig->method('getValueInt')
63-
->with('files_trashbin', 'background_job_expire_trash_offset', 0)
71+
->with(Application::APP_ID, ExpireTrash::OFFSET_CONFIG_KEY_NAME, 0)
6472
->willReturn(0);
6573

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

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

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

‎lib/private/User/Manager.php‎

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

652652
/**
653-
* Getting all userIds that have a listLogin value requires checking the
653+
* Getting all userIds that have a lastLogin value requires checking the
654654
* value in php because on oracle you cannot use a clob in a where clause,
655655
* preventing us from doing a not null or length(value) > 0 check.
656656
*
@@ -828,19 +828,19 @@ public function getDisplayNameCache(): DisplayNameCache {
828828
return $this->displayNameCache;
829829
}
830830

831-
/**
832-
* Gets the list of users sorted by lastLogin, from most recent to least recent
833-
*
834-
* @param int $offset from which offset to fetch
835-
* @return \Iterator<IUser> list of user IDs
836-
* @since 30.0.0
837-
*/
838-
public function getSeenUsers(int $offset = 0): \Iterator {
839-
$limit = 1000;
831+
public function getSeenUsers(int $offset = 0, ?int $limit = null): \Iterator {
832+
$maxBatchSize = 1000;
840833

841834
do {
842-
$userIds = $this->getSeenUserIds($limit, $offset);
843-
$offset += $limit;
835+
if ($limit !== null) {
836+
$batchSize = min($limit, $maxBatchSize);
837+
$limit -= $batchSize;
838+
} else {
839+
$batchSize = $maxBatchSize;
840+
}
841+
842+
$userIds = $this->getSeenUserIds($batchSize, $offset);
843+
$offset += $batchSize;
844844

845845
foreach ($userIds as $userId) {
846846
foreach ($this->backends as $backend) {
@@ -851,6 +851,6 @@ public function getSeenUsers(int $offset = 0): \Iterator {
851851
}
852852
}
853853
}
854-
} while (count($userIds) === $limit);
854+
} while (count($userIds) === $batchSize && $limit !== 0);
855855
}
856856
}

‎lib/public/IUserManager.php‎

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

0 commit comments

Comments
 (0)