From 86b63410989314ae69651d2d3c2ad3571f882cb6 Mon Sep 17 00:00:00 2001 From: Clement Date: Tue, 16 Apr 2019 19:40:59 -0700 Subject: [PATCH] Add error reporting + Make temp dir erase safer --- assignment-client/src/assets/AssetServer.cpp | 91 ++++++++++++------- .../src/assets/BakeAssetTask.cpp | 54 +++++------ libraries/shared/src/PathUtils.cpp | 24 +++++ libraries/shared/src/PathUtils.h | 1 + 4 files changed, 102 insertions(+), 68 deletions(-) diff --git a/assignment-client/src/assets/AssetServer.cpp b/assignment-client/src/assets/AssetServer.cpp index 297a622c25..502cf15aa2 100644 --- a/assignment-client/src/assets/AssetServer.cpp +++ b/assignment-client/src/assets/AssetServer.cpp @@ -1314,27 +1314,57 @@ void AssetServer::handleFailedBake(QString originalAssetHash, QString assetPath, void AssetServer::handleCompletedBake(QString originalAssetHash, QString originalAssetPath, QString bakedTempOutputDir) { + auto reportCompletion = [this, originalAssetPath, originalAssetHash](bool errorCompletingBake, + QString errorReason, + QString redirectTarget) { + auto type = assetTypeForFilename(originalAssetPath); + auto currentTypeVersion = currentBakeVersionForAssetType(type); + + AssetMeta meta; + meta.bakeVersion = currentTypeVersion; + meta.failedLastBake = errorCompletingBake; + meta.redirectTarget = redirectTarget; + + if (errorCompletingBake) { + qWarning() << "Could not complete bake for" << originalAssetHash; + meta.lastBakeErrors = errorReason; + } + + writeMetaFile(originalAssetHash, meta); + + _pendingBakes.remove(originalAssetHash); + }; + bool errorCompletingBake { false }; QString errorReason; + QString redirectTarget; qDebug() << "Completing bake for " << originalAssetHash; - - + // Find the directory containing the baked content QDir outputDir(bakedTempOutputDir); + QString outputDirName = outputDir.dirName(); auto directories = outputDir.entryList(QDir::Dirs | QDir::NoDotAndDotDot); - assert(directories.size() == 1); QString bakedDirectoryPath; for (const auto& dirName : directories) { outputDir.cd(dirName); if (outputDir.exists("baked") && outputDir.exists("original")) { bakedDirectoryPath = outputDir.filePath("baked"); + break; } outputDir.cdUp(); } + if (bakedDirectoryPath.isEmpty()) { + errorCompletingBake = true; + errorReason = "Failed to find baking output"; - assert(!bakedDirectoryPath.isEmpty()); + // Cleanup temporary output directory + PathUtils::deleteMyTemporaryDir(outputDirName); + reportCompletion(errorCompletingBake, errorReason, redirectTarget); + return; + } + // Compile list of all the baked files QDirIterator it(bakedDirectoryPath, QDirIterator::Subdirectories); QVector bakedFilePaths; while (it.hasNext()) { @@ -1343,9 +1373,17 @@ void AssetServer::handleCompletedBake(QString originalAssetHash, QString origina bakedFilePaths.push_back(it.filePath()); } } + if (bakedFilePaths.isEmpty()) { + errorCompletingBake = true; + errorReason = "Baking output has no files"; + + // Cleanup temporary output directory + PathUtils::deleteMyTemporaryDir(outputDirName); + reportCompletion(errorCompletingBake, errorReason, redirectTarget); + return; + } QDir bakedDirectory(bakedDirectoryPath); - QString redirectTarget; for (auto& filePath : bakedFilePaths) { // figure out the hash for the contents of this file @@ -1359,7 +1397,7 @@ void AssetServer::handleCompletedBake(QString originalAssetHash, QString origina qDebug() << "Failed to open baked file: " << filePath; // stop handling this bake, we couldn't open one of the files for reading errorCompletingBake = true; - errorReason = "Failed to finalize bake"; + errorReason = "Could not open baked file " + file.fileName(); break; } @@ -1368,7 +1406,7 @@ void AssetServer::handleCompletedBake(QString originalAssetHash, QString origina if (!hasher.addData(&file)) { // stop handling this bake, couldn't hash the contents of the file errorCompletingBake = true; - errorReason = "Failed to finalize bake"; + errorReason = "Could not hash data for " + file.fileName(); break; } @@ -1388,13 +1426,15 @@ void AssetServer::handleCompletedBake(QString originalAssetHash, QString origina // setup the mapping for this bake file auto relativeFilePath = bakedDirectory.relativeFilePath(filePath); - qDebug() << "Relative file path is: " << relativeFilePath; QString bakeMapping = getBakeMapping(originalAssetHash, relativeFilePath); // Check if this is the file we should redirect to when someone asks for the original asset if ((relativeFilePath.endsWith(".baked.fst", Qt::CaseInsensitive) && originalAssetPath.endsWith(".fbx")) || (relativeFilePath.endsWith(".texmeta.json", Qt::CaseInsensitive) && !originalAssetPath.endsWith(".fbx"))) { + if (!redirectTarget.isEmpty()) { + qWarning() << "Found multiple baked redirect target for" << originalAssetPath; + } redirectTarget = bakeMapping; } @@ -1403,41 +1443,22 @@ void AssetServer::handleCompletedBake(QString originalAssetHash, QString origina qDebug() << "Failed to set mapping"; // stop handling this bake, couldn't add a mapping for this bake file errorCompletingBake = true; - errorReason = "Failed to finalize bake"; + errorReason = "Failed to set mapping for baked file " + file.fileName(); break; } qDebug() << "Added" << bakeMapping << "for bake file" << bakedFileHash << "from bake of" << originalAssetHash; } - for (auto& filePath : bakedFilePaths) { - QFile file(filePath); - if (!file.remove()) { - qWarning() << "Failed to remove temporary file:" << filePath; - } - } - if (!QDir(bakedTempOutputDir).rmdir(".")) { - qWarning() << "Failed to remove temporary directory:" << bakedTempOutputDir; + + if (redirectTarget.isEmpty()) { + errorCompletingBake = true; + errorReason = "Could not find root file for baked output"; } - auto type = assetTypeForFilename(originalAssetPath); - auto currentTypeVersion = currentBakeVersionForAssetType(type); - - assert(!redirectTarget.isEmpty()); - - AssetMeta meta; - meta.bakeVersion = currentTypeVersion; - meta.failedLastBake = errorCompletingBake; - meta.redirectTarget = redirectTarget; - - if (errorCompletingBake) { - qWarning() << "Could not complete bake for" << originalAssetHash; - meta.lastBakeErrors = errorReason; - } - - writeMetaFile(originalAssetHash, meta); - - _pendingBakes.remove(originalAssetHash); + // Cleanup temporary output directory + PathUtils::deleteMyTemporaryDir(outputDirName); + reportCompletion(errorCompletingBake, errorReason, redirectTarget); } void AssetServer::handleAbortedBake(QString originalAssetHash, QString assetPath) { diff --git a/assignment-client/src/assets/BakeAssetTask.cpp b/assignment-client/src/assets/BakeAssetTask.cpp index 7ca87fe4e3..99fff4f180 100644 --- a/assignment-client/src/assets/BakeAssetTask.cpp +++ b/assignment-client/src/assets/BakeAssetTask.cpp @@ -36,21 +36,6 @@ BakeAssetTask::BakeAssetTask(const AssetUtils::AssetHash& assetHash, const Asset }); } -void cleanupTempFiles(QString tempOutputDir, std::vector files) { - for (const auto& filename : files) { - QFile f { filename }; - if (!f.remove()) { - qDebug() << "Failed to remove:" << filename; - } - } - if (!tempOutputDir.isEmpty()) { - QDir dir { tempOutputDir }; - if (!dir.rmdir(".")) { - qDebug() << "Failed to remove temporary directory:" << tempOutputDir; - } - } -}; - void BakeAssetTask::run() { if (_isBaking.exchange(true)) { qWarning() << "Tried to start bake asset task while already baking"; @@ -59,11 +44,24 @@ void BakeAssetTask::run() { // Make a new temporary directory for the Oven to work in QString tempOutputDir = PathUtils::generateTemporaryDir(); + QString tempOutputDirName = QDir(tempOutputDir).dirName(); + if (tempOutputDir.isEmpty()) { + QString errors = "Could not create temporary working directory"; + emit bakeFailed(_assetHash, _assetPath, errors); + PathUtils::deleteMyTemporaryDir(tempOutputDirName); + return; + } // Copy file to bake the temporary dir and give a name the oven can work with auto assetName = _assetPath.split("/").last(); auto tempAssetPath = tempOutputDir + "/" + assetName; - QFile::copy(_filePath, tempAssetPath); + auto sucess = QFile::copy(_filePath, tempAssetPath); + if (!sucess) { + QString errors = "Couldn't copy file to bake to temporary directory"; + emit bakeFailed(_assetHash, _assetPath, errors); + PathUtils::deleteMyTemporaryDir(tempOutputDirName); + return; + } auto base = QFileInfo(QCoreApplication::applicationFilePath()).absoluteDir(); QString path = base.absolutePath() + "/oven"; @@ -79,30 +77,23 @@ void BakeAssetTask::run() { QEventLoop loop; connect(_ovenProcess.get(), static_cast(&QProcess::finished), - this, [&loop, this, tempOutputDir, tempAssetPath](int exitCode, QProcess::ExitStatus exitStatus) { + this, [&loop, this, tempOutputDir, tempAssetPath, tempOutputDirName](int exitCode, QProcess::ExitStatus exitStatus) { qDebug() << "Baking process finished: " << exitCode << exitStatus; if (exitStatus == QProcess::CrashExit) { + PathUtils::deleteMyTemporaryDir(tempOutputDirName); if (_wasAborted) { emit bakeAborted(_assetHash, _assetPath); } else { QString errors = "Fatal error occurred while baking"; emit bakeFailed(_assetHash, _assetPath, errors); } - if (!QDir(tempOutputDir).rmdir(".")) { - qWarning() << "Failed to remove temporary directory:" << tempOutputDir; - } } else if (exitCode == OVEN_STATUS_CODE_SUCCESS) { - // Remove temp copy of the original asset - QFile::remove(tempAssetPath); - emit bakeComplete(_assetHash, _assetPath, tempOutputDir); } else if (exitStatus == QProcess::NormalExit && exitCode == OVEN_STATUS_CODE_ABORT) { _wasAborted.store(true); + PathUtils::deleteMyTemporaryDir(tempOutputDirName); emit bakeAborted(_assetHash, _assetPath); - if (!QDir(tempOutputDir).rmdir(".")) { - qWarning() << "Failed to remove temporary directory:" << tempOutputDir; - } } else { QString errors; if (exitCode == OVEN_STATUS_CODE_FAIL) { @@ -116,10 +107,8 @@ void BakeAssetTask::run() { errors = "Unknown error occurred while baking"; } } + PathUtils::deleteMyTemporaryDir(tempOutputDirName); emit bakeFailed(_assetHash, _assetPath, errors); - if (!QDir(tempOutputDir).rmdir(".")) { - qWarning() << "Failed to remove temporary directory:" << tempOutputDir; - } } loop.quit(); @@ -128,12 +117,11 @@ void BakeAssetTask::run() { qDebug() << "Starting oven for " << _assetPath; _ovenProcess->start(path, args, QIODevice::ReadOnly); qDebug() << "Running:" << path << args; - if (!_ovenProcess->waitForStarted(-1)) { + if (!_ovenProcess->waitForStarted()) { + PathUtils::deleteMyTemporaryDir(tempOutputDirName); + QString errors = "Oven process failed to start"; emit bakeFailed(_assetHash, _assetPath, errors); - if (!QDir(tempOutputDir).rmdir(".")) { - qWarning() << "Failed to remove temporary directory:" << tempOutputDir; - } return; } diff --git a/libraries/shared/src/PathUtils.cpp b/libraries/shared/src/PathUtils.cpp index 60b426e46d..be60533406 100644 --- a/libraries/shared/src/PathUtils.cpp +++ b/libraries/shared/src/PathUtils.cpp @@ -185,6 +185,30 @@ QString PathUtils::generateTemporaryDir() { return ""; } +bool PathUtils::deleteMyTemporaryDir(QString dirName) { + QDir rootTempDir = QDir::tempPath(); + + QString appName = qApp->applicationName(); + QRegularExpression re { "^" + QRegularExpression::escape(appName) + "\\-(?\\d+)\\-(?\\d+)$" }; + + auto match = re.match(dirName); + auto pid = match.capturedRef("pid").toLongLong(); + + if (match.hasMatch() && rootTempDir.exists(dirName) && pid == qApp->applicationPid()) { + auto absoluteDirPath = QDir(rootTempDir.absoluteFilePath(dirName)); + + bool success = absoluteDirPath.removeRecursively(); + if (success) { + qDebug() << " Removing temporary directory: " << absoluteDirPath.absolutePath(); + } else { + qDebug() << " Failed to remove temporary directory: " << absoluteDirPath.absolutePath(); + } + return success; + } + + return false; +} + // Delete all temporary directories for an application int PathUtils::removeTemporaryApplicationDirs(QString appName) { if (appName.isNull()) { diff --git a/libraries/shared/src/PathUtils.h b/libraries/shared/src/PathUtils.h index 0096cb6c90..cac5c58ca4 100644 --- a/libraries/shared/src/PathUtils.h +++ b/libraries/shared/src/PathUtils.h @@ -56,6 +56,7 @@ public: static QString getAppLocalDataFilePath(const QString& filename); static QString generateTemporaryDir(); + static bool deleteMyTemporaryDir(QString dirName); static int removeTemporaryApplicationDirs(QString appName = QString::null);