From cddfdb7d1d0d8492098640ca282ebac7ca5697ac Mon Sep 17 00:00:00 2001 From: James Turner Date: Thu, 3 Dec 2020 16:40:48 +0000 Subject: [PATCH] TerraSync: stronger fix for handling 0-byte files Change logic so we create an empty file for such cases, i.e exactly matching the repository. This simplifies logic in downstream code, compared with not creating a local file. Add a test-case to cover this Modify TerraSync to detect a failure of Airports_archive downloading, and fall back to file-by-file updating. --- simgear/io/HTTPRepository.cxx | 101 ++++++++++++++++++------------ simgear/io/test_repository.cxx | 7 +++ simgear/scene/tsync/terrasync.cxx | 9 +++ 3 files changed, 76 insertions(+), 41 deletions(-) diff --git a/simgear/io/HTTPRepository.cxx b/simgear/io/HTTPRepository.cxx index 147b7d62..88920ead 100644 --- a/simgear/io/HTTPRepository.cxx +++ b/simgear/io/HTTPRepository.cxx @@ -978,55 +978,74 @@ HTTPRepository::failure() const { pathInRepo = _directory->absolutePath(); pathInRepo.append(fileName); - - sha1_init(&hashContext); } protected: - void gotBodyData(const char *s, int n) override { - if (!file.get()) { - file.reset(new SGBinaryFile(pathInRepo)); - if (!file->open(SG_IO_OUT)) { - SG_LOG(SG_TERRASYNC, SG_WARN, - "unable to create file " << pathInRepo); - _directory->repository()->http->cancelRequest( - this, "Unable to create output file:" + pathInRepo.utf8Str()); - } + void gotBodyData(const char* s, int n) override + { + if (!file.get()) { + const bool ok = createOutputFile(); + if (!ok) { + _directory->repository()->http->cancelRequest( + this, "Unable to create output file:" + pathInRepo.utf8Str()); + } + } + + sha1_write(&hashContext, s, n); + file->write(s, n); } - sha1_write(&hashContext, s, n); - file->write(s, n); - } + bool createOutputFile() + { + file.reset(new SGBinaryFile(pathInRepo)); + if (!file->open(SG_IO_OUT)) { + SG_LOG(SG_TERRASYNC, SG_WARN, + "unable to create file " << pathInRepo); + return false; + } - void onDone() override { - if (file) { - file->close(); - } - - if (responseCode() == 200) { - std::string hash = - strutils::encodeHex(sha1_result(&hashContext), HASH_LENGTH); - _directory->didUpdateFile(fileName, hash, contentSize()); - } else if (responseCode() == 404) { - SG_LOG(SG_TERRASYNC, SG_WARN, - "terrasync file not found on server: " - << fileName << " for " << _directory->absolutePath()); - _directory->didFailToUpdateFile( - fileName, HTTPRepository::REPO_ERROR_FILE_NOT_FOUND); - } else { - SG_LOG(SG_TERRASYNC, SG_WARN, - "terrasync file download error on server: " - << fileName << " for " << _directory->absolutePath() - << "\n\tserver responded: " << responseCode() << "/" - << responseReason()); - _directory->didFailToUpdateFile(fileName, - HTTPRepository::REPO_ERROR_HTTP); - // should we every retry here? + sha1_init(&hashContext); + return true; } - _directory->repository()->finishedRequest( - this, HTTPRepoPrivate::RequestFinish::Done); - } + void onDone() override + { + const bool is200Response = (responseCode() == 200); + if (!file && is200Response) { + // if the server defines a zero-byte file, we will never call + // gotBodyData, so create the file here + // this ensures all the logic below works as expected + createOutputFile(); + } + + if (file) { + file->close(); + } + + if (is200Response) { + std::string hash = + strutils::encodeHex(sha1_result(&hashContext), HASH_LENGTH); + _directory->didUpdateFile(fileName, hash, contentSize()); + } else if (responseCode() == 404) { + SG_LOG(SG_TERRASYNC, SG_WARN, + "terrasync file not found on server: " + << fileName << " for " << _directory->absolutePath()); + _directory->didFailToUpdateFile( + fileName, HTTPRepository::REPO_ERROR_FILE_NOT_FOUND); + } else { + SG_LOG(SG_TERRASYNC, SG_WARN, + "terrasync file download error on server: " + << fileName << " for " << _directory->absolutePath() + << "\n\tserver responded: " << responseCode() << "/" + << responseReason()); + _directory->didFailToUpdateFile(fileName, + HTTPRepository::REPO_ERROR_HTTP); + // should we every retry here? + } + + _directory->repository()->finishedRequest( + this, HTTPRepoPrivate::RequestFinish::Done); + } void onFail() override { HTTPRepository::ResultCode code = HTTPRepository::REPO_ERROR_SOCKET; diff --git a/simgear/io/test_repository.cxx b/simgear/io/test_repository.cxx index a5e5b762..6e3f86d4 100644 --- a/simgear/io/test_repository.cxx +++ b/simgear/io/test_repository.cxx @@ -31,6 +31,10 @@ using TestApi = simgear::HTTP::TestApi; std::string dataForFile(const std::string& parentName, const std::string& name, int revision) { + if (name == "zeroByteFile") { + return {}; + } + std::ostringstream os; // random content but which definitely depends on our tree location // and revision. @@ -446,6 +450,7 @@ void testBasicClone(HTTP::Client* cl) verifyFileState(p, "fileA"); verifyFileState(p, "dirA/subdirA/fileAAA"); verifyFileState(p, "dirC/subdirA/subsubA/fileCAAA"); + verifyFileState(p, "dirA/subdirA/zeroByteFile"); global_repo->findEntry("fileA")->revision++; global_repo->findEntry("dirB/subdirA/fileBAA")->revision++; @@ -911,6 +916,8 @@ int main(int argc, char* argv[]) global_repo->defineFile("dirA/fileAC"); global_repo->defineFile("dirA/subdirA/fileAAA"); global_repo->defineFile("dirA/subdirA/fileAAB"); + global_repo->defineFile("dirA/subdirA/zeroByteFile"); + global_repo->defineFile("dirB/subdirA/fileBAA"); global_repo->defineFile("dirB/subdirA/fileBAB"); global_repo->defineFile("dirB/subdirA/fileBAC"); diff --git a/simgear/scene/tsync/terrasync.cxx b/simgear/scene/tsync/terrasync.cxx index e9702fad..a2af702d 100644 --- a/simgear/scene/tsync/terrasync.cxx +++ b/simgear/scene/tsync/terrasync.cxx @@ -572,6 +572,15 @@ void SGTerraSync::WorkerThread::updateSyncSlot(SyncSlot &slot) notFound(slot.currentItem); } else if (res != HTTPRepository::REPO_NO_ERROR) { fail(slot.currentItem); + + // in case the Airports_archive download fails, create the + // directory, so that next sync, we do a manual sync + if ((slot.currentItem._type == SyncItem::AirportData) && slot.isNewDirectory) { + SG_LOG(SG_TERRASYNC, SG_ALERT, "Failed to download Airports_archive, will download discrete files next time"); + simgear::Dir d(_local_dir + "/Airports"); + d.create(0755); + _completedTiles.erase(slot.currentItem._dir); + } } else { updated(slot.currentItem, slot.isNewDirectory); SG_LOG(SG_TERRASYNC, SG_DEBUG, "sync of " << slot.repository->baseUrl() << " finished ("