fix: Throw instead of yielding nothing when listing local directories #43787
Conversation
Signed-off-by: Julius Härtl <jus@bitgrid.net>
juliusknorr
requested review from
icewind1991,
Altahrim,
a team and
nfebe
and removed request for
a team
| @@ -898,6 +899,11 @@ public function writeStream(string $path, $stream, int $size = null): int { | |||
|
|
|||
| public function getDirectoryContent($directory): \Traversable { | |||
| $dh = $this->opendir($directory); | |||
There was a problem hiding this comment.
Question:
| $dh = $this->opendir($directory); | |
| try { | |
| $dh = $this->opendir($directory); | |
| } catch(\Exception $e) { | |
| // Throw and take other actions? | |
| } |
Can't be caught? To capture the actual failure/error? (That might required an update to method opendir if the reading functions through meaningful error messages.
There was a problem hiding this comment.
If that method throws the exception should already propagate, don't think we need to wrap that.
The local storage implementation would already log an error but return false which just wasn't handled before.
skjnldsv
added
4. to release
|
/backport to stable28 |
|
/backport to stable27 |
|
/backport to stable26 |
|
/backport to stable25 |
|
/backport to stable24 |
|
/backport to stable23 |
|
/backport to stable24 |
|
/backport to stable23 |
| if ($dh === false) { | ||
| throw new StorageNotAvailableException('Directory listing failed'); | ||
| } | ||
|
|
There was a problem hiding this comment.
Potentially impacts all storage subsystems, since you put the change into Common.php . In particular, the sftp storage inherits the same implementation and breaks on sub-folders without sufficient permissions. Please revert that patch or move it to Local.php
I've been poking around in code trying to replicate scenarios where out of a sudden file ids were changed. Due to one case where we have seen this with a temporary storage failure I tried making the __groupfolders directory not readable by changing filesystem permissions. Now when we hit file scanning logic we may end up calling the directory listing from the Local storage class which on an error does not fail but instead yields nothing which means that the scanner will handle the given directory as if it would be empty.
Note I'm not sure this actually fixes the issue and also not what could cause a scan on the folder out of a sudden (in testing i manually set the size to -1 to trigger this).
@icewind1991 Maybe you can have a look, i think from a code perspective is sensible and should avoid unnecessary file cache entry drops if the opendir fails. However I also cannot really exclude possible side-effects of a scanner then not removing a missing directory on the filesystem.
Related to nextcloud/groupfolders#1468
I also remember issues in the past where the size was always pending on some group folders (not sure if that was fixed meanwhile), but could then probably trigger such cases if the scan never finishes or happens regularly.
Checklist