Skip to content

Commit

Permalink
MDL-77149 core_files: Network filesystem (Amazon EFS) can warn
Browse files Browse the repository at this point in the history
If you delete a file with a hash and then create another file with
the same hash, sometimes on EFS filesystems while trying to create
the new file, it returns true to the file_exists check even though
the file doesn't exist, but then fails other calls.

This change makes Moodle tolerate that behaviour.
  • Loading branch information
sammarshallou committed Feb 21, 2023
1 parent 3312a68 commit dadceea
Showing 1 changed file with 42 additions and 4 deletions.
46 changes: 42 additions & 4 deletions lib/filestorage/file_system_filedir.php
Expand Up @@ -352,8 +352,9 @@ public function add_file_from_path($pathname, $contenthash = null) {

$newfile = true;

if (file_exists($hashfile)) {
if (filesize($hashfile) === $filesize) {
$hashsize = self::check_file_exists_and_get_size($hashfile);
if ($hashsize !== null) {
if ($hashsize === $filesize) {
return array($contenthash, $filesize, false);
}
if (file_storage::hash_from_path($hashfile) === $contenthash) {
Expand Down Expand Up @@ -411,6 +412,42 @@ public function add_file_from_path($pathname, $contenthash = null) {
return array($contenthash, $filesize, $newfile);
}

/**
* Checks if the file exists and gets its size. This function avoids a specific issue with
* networked file systems if they incorrectly report the file exists, but then decide it doesn't
* as soon as you try to get the file size.
*
* @param string $hashfile File to check
* @return int|null Null if the file does not exist, or the result of filesize(), or -1 if error
*/
protected static function check_file_exists_and_get_size(string $hashfile): ?int {
if (!file_exists($hashfile)) {
// The file does not exist, return null.
return null;
}

// In some networked file systems, it's possible that file_exists will return true when
// the file doesn't exist (due to caching), but filesize will then return false because
// it doesn't exist.
$hashsize = @filesize($hashfile);
if ($hashsize !== false) {
// We successfully got a file size. Return it.
return $hashsize;
}

// If we can't get the filesize, let's check existence again to see if we really
// for sure think it exists.
clearstatcache();
if (!file_exists($hashfile)) {
// The file doesn't exist any more, so return null.
return null;
}

// It still thinks the file exists, but filesize failed, so we had better return an invalid
// value for filesize.
return -1;
}

/**
* Add a file with the supplied content to the file system.
*
Expand All @@ -433,8 +470,9 @@ public function add_file_from_string($content) {

$newfile = true;

if (file_exists($hashfile)) {
if (filesize($hashfile) === $filesize) {
$hashsize = self::check_file_exists_and_get_size($hashfile);
if ($hashsize !== null) {
if ($hashsize === $filesize) {
return array($contenthash, $filesize, false);
}
if (file_storage::hash_from_path($hashfile) === $contenthash) {
Expand Down

0 comments on commit dadceea

Please sign in to comment.