Skip to content
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

fix(timeout): compare creation time to now - timeout #497

Merged
merged 3 commits into from
Feb 13, 2025

Conversation

max-nextcloud
Copy link
Collaborator

Fixes #264.

@max-nextcloud max-nextcloud marked this pull request as draft January 20, 2025 15:46
@max-nextcloud
Copy link
Collaborator Author

Haven't been able to test this yet myself.

@juliusknorr
Copy link
Member

Please also check if we can cover this with tests, we have something around expiry in

public function testLockUserExpire() {
\OCP\Server::get(IConfig::class)->setAppValue(Application::APP_ID, ConfigService::LOCK_TIMEOUT, 30);
$file = $this->loginAndGetUserFolder(self::TEST_USER1)
->newFile('test-file-expire', 'AAA');
$this->shareFileWithUser($file, self::TEST_USER1, self::TEST_USER2);
$this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::TEST_USER1));
$file->putContent('BBB');
/** @var File */
$file = $this->loginAndGetUserFolder(self::TEST_USER2)
->get('test-file-expire');
try {
$file->putContent('CCC');
$this->fail('Expected to throw a ManuallyLockedException');
} catch (ManuallyLockedException $e) {
self::assertInstanceOf(ManuallyLockedException::class, $e);
self::assertEquals('BBB', $file->getContent());
}
$this->toTheFuture(3600);
$file->putContent('CCC');
self::assertEquals('CCC', $file->getContent());
}
at least, but maybe that test does not cover it properly

@juliusknorr
Copy link
Member

Makes sense code wide 👍

@max-nextcloud max-nextcloud marked this pull request as ready for review January 27, 2025 10:34
@max-nextcloud max-nextcloud self-assigned this Jan 27, 2025
@max-nextcloud
Copy link
Collaborator Author

Please also check if we can cover this with tests

I added an assertion to the test but it's failing. Maybe we can investigate together in the coming days. Locally the test fails for me before it even gets to this point. Actually I think I will turn this into two tests.

@max-nextcloud max-nextcloud force-pushed the fix/timeout-handling branch 2 times, most recently from db98b9d to a41ab3c Compare February 5, 2025 09:41
@max-nextcloud
Copy link
Collaborator Author

@juliusknorr I think the failing static test is somehow a sideeffect of adding logging to lib/Service/LockService.php - but I don't understand how that would come about.
Could you have another look?

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Fixes #264.

Signed-off-by: Max <[email protected]>
@max-nextcloud max-nextcloud force-pushed the fix/timeout-handling branch 5 times, most recently from 805ec40 to a6efda4 Compare February 13, 2025 08:47
Comment on lines 392 to 407

/**
* @param string $message
* @param bool $trace
* @param array $serializable
*/
private function notice(string $message, bool $trace = false, array $serializable = []) {
$opts = ['app' => 'files_lock'];
if ($trace) {
$opts['exception'] = new HintException($message, json_encode($serializable));
} elseif (!empty($serializable)) {
$message .= ' -- ' . json_encode($serializable);
}
$this->logger->notice('[notice] ' . $message, $opts);
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly took this from the TLogger trait - but i don't know how the Psr\Log\LoggerInteface works:

  • do we still need to prefix the message with [notice] ?
  • do the $opts have the same effect they used to have in the TLogger?

Signed-off-by: Max <[email protected]>
@juliusknorr juliusknorr merged commit 15123c9 into main Feb 13, 2025
48 checks passed
@juliusknorr juliusknorr deleted the fix/timeout-handling branch February 13, 2025 15:46
@juliusknorr
Copy link
Member

/backport to stable31

@juliusknorr
Copy link
Member

/backport to stable30

@juliusknorr
Copy link
Member

/backport to stable29

@juliusknorr
Copy link
Member

/backport to stable28

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ☑️ Done
Development

Successfully merging this pull request may close these issues.

Potential Logic Issue in GetLocksOlderThan Methods Data Retrieval.
2 participants