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

Updating an asset can cause stat issues #467

Open
lekoala opened this issue Aug 31, 2021 · 6 comments
Open

Updating an asset can cause stat issues #467

lekoala opened this issue Aug 31, 2021 · 6 comments

Comments

@lekoala
Copy link
Contributor

lekoala commented Aug 31, 2021

I have discovered a potential issue or at least something that is really not clear when working on one of my module

I'm trying to update the content from a file using the setFromStream method like this

https://github.com/lekoala/silverstripe-encrypt/blob/22b305c1301d6f67fd3d57df1b04d7bee6308c82/src/EncryptedDBFile.php#L108

I see it's updating the filename based on the file results array... but since the content has been updated, the hash is updated as well. Which means the file is moved to a new folder. All that is fine, except that the FileHash field in my File table is not updated and it seems that the file could not be found again after being updated (it's there on the filesystem in the new hash folder, but it's shown as not found by the uploader and trying to manipulate the file in php results in strange errors).

Did I miss something? I expect the setFromStream method to update the hash as well if necessary. I've tried setting the hash manually (something like $file->FileHash = $newHash) but it doesn't seem to get saved.
Currently my solution is to pass along the previous hash to the setFromStream method (which also prevent moving the file around for no reason).

EDIT: i've changed the title of the issue, please read below

@lekoala
Copy link
Contributor Author

lekoala commented Sep 1, 2021

Actually forcing the hash is not a solution either since we have this

if ($fs->has($originalFileID)) {
/** @var FileHashingService $hasher */
$hasher = Injector::inst()->get(FileHashingService::class);
$actualHash = $hasher->computeFromFile($originalFileID, $fs);
// If the hash of the file doesn't match we return false, because we want to keep looking.
return $hasher->compare($actualHash, $hash);

which would cause the file to be marked as "not found" even if it's there.

So back to square one, i need to update the hash. Currently I have the following behaviour:

  • I update the content of the file with a new stream
  • The file gets moved due to the new hash being computed
  • If i trigger a write, the old assets (db record) is marked as deleted as asked in onBeforeWrite by AssetControlExtension
  • When trying to truncate the directory, the old assets is not found on the filesystem, because it was moved => we have a an exception like this
    [Emergency] Uncaught RuntimeException: SplFileInfo::getType(): Lstat failed for ...\assets.protected\Invoice/329/e79c8f8d64\Document-v8.pdf

I'm not sure how we end up in a situation where truncating the directory tries to find the old asset but if i comment the truncateDir line, it works!

The full trace is as follow

SplFileInfo->getType()
Local.php:509
League\Flysystem\Adapter\Local->mapFileInfo(Document-v9.pdf)
Local.php:452
League\Flysystem\Adapter\Local->normalizeFileInfo(Document-v9.pdf)
Local.php:288
League\Flysystem\Adapter\Local->listContents(Invoice/329/e79c8f8d64, )
Filesystem.php:272
League\Flysystem\Filesystem->listContents(Invoice/329/e79c8f8d64)
FlysystemAssetStore.php:756
SilverStripe\Assets\Flysystem\FlysystemAssetStore->truncateDirectory(Invoice/329/e79c8f8d64, League\Flysystem\Filesystem)
FlysystemAssetStore.php:740
SilverStripe\Assets\Flysystem\FlysystemAssetStore->deleteFromFileStore(SilverStripe\Assets\FilenameParsing\ParsedFileID, League\Flysystem\Filesystem, SilverStripe\Assets\FilenameParsing\FileIDHelperResolutionStrategy)
FlysystemAssetStore.php:6037
...

@lekoala
Copy link
Contributor Author

lekoala commented Sep 1, 2021

Actually the issue might be in flysystem itself, for reference

thephpleague/flysystem#1352

@lekoala
Copy link
Contributor Author

lekoala commented Sep 1, 2021

Rewriting the truncateDirectory method like this "fixes" the issue. The directory won't be truncated if the deleted file was the only in there but it's better than having the whole thing crashing.
I did not find a good way to clear cache except by using clearstatcache in listContents in the loop, which sounds like a massive performance issue at first sight.

    /**
     * Clear directory if it's empty
     *
     * @param string $dirname Name of directory
     * @param Filesystem $filesystem
     */
    protected function truncateDirectory($dirname, Filesystem $filesystem)
    {
        // Check valid dirname
        if (!$dirname || !ltrim($dirname, '.')) {
            return;
        }
        // We keep empty dir
        if ($this->config()->get('keep_empty_dirs')) {
            return;
        }
        // listContents can throw RuntimeExceptions due to stat cache
        try {
            $listContents = $filesystem->listContents($dirname);
            if ($listContents) {
                return;
            }
        } catch (RuntimeException $ex) {
            // Ignore 'Uncaught RuntimeException: SplFileInfo::getType(): Lstat failed for' errors
            return;
        }
        $filesystem->deleteDir($dirname);
        $this->truncateDirectory(dirname($dirname), $filesystem);
    }

@lekoala lekoala changed the title Updating an asset does not update the hash properly Updating an asset can cause stat issues Sep 1, 2021
@emteknetnz
Copy link
Member

So, basically calling setFromStream causes your file to vanish?

@lekoala
Copy link
Contributor Author

lekoala commented Sep 2, 2021

@emteknetnz not quite, the file is still there on the filesystem, but moved to a folder with the new hash. In fact, everything works fine if you don't truncate the directory, the hash is updated properly and silverstripe finds it back.
but when you truncate the directory, you get an exception due to the listContents function returning the same array of files (even if it's been moved in the meantime)

the scenario is this:

  • getVariants triggers the listContents function => the file is still there and the function works properly
  • updating the file moves it in a new folder
  • truncateDirectory triggers the listContents function => the file is still reported to be there even if moved away. Flysystem run a getType function which triggers a lstat error
    (skipping this step prevents the error, or catching the exception as explained above)

I don't know if it's only on my local windows machine or also on a linux/mac server.

@lekoala
Copy link
Contributor Author

lekoala commented Sep 2, 2021

if you want to replicate the issue, you can try with my module lekoala/silverstyripe-encrypt and comment the lines where i set keep_empty_dirs to true. Define a DataObject with a has_one File and run in onBeforeWrite

if($this->FileID) { $this->File()->encryptFileIfNeeded(); }

https://github.com/lekoala/silverstripe-encrypt/blob/67655ff6fae0346d65451944c774d897499fc81d/src/EncryptedDBFile.php#L127-L138

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants