From 2268e01df95f3aa20a14e60433a9673c8d18fc75 Mon Sep 17 00:00:00 2001 From: Pawel Olejniczak Date: Wed, 29 Sep 2021 02:13:27 +0200 Subject: [PATCH] [CP-599] Add crc hash to the last chunk of sent file CRC was counted in one go before starting to send chunks of the file, which, in the case of a large file, caused communication to stop for a long while. In the current implementation, CRC is counted incrementally while sending individual file chunks. It is then appended to the last chunk of the file. --- .../endpoints/filesystem/FileContext.cpp | 60 ++++++------------- .../endpoints/filesystem/FileOperations.cpp | 32 ++-------- .../filesystem/FilesystemEndpoint.cpp | 21 ++++--- .../endpoints/filesystem/FileContext.hpp | 14 ++--- .../endpoints/filesystem/FileOperations.hpp | 14 +++-- test/harness | 2 +- 6 files changed, 52 insertions(+), 91 deletions(-) diff --git a/module-services/service-desktop/endpoints/filesystem/FileContext.cpp b/module-services/service-desktop/endpoints/filesystem/FileContext.cpp index 0e71d44d350a4c27785e6a66f6b8b45cbef52460..1a056d3dc33cb6dd53359fe9eface3bc2f97d3cf 100644 --- a/module-services/service-desktop/endpoints/filesystem/FileContext.cpp +++ b/module-services/service-desktop/endpoints/filesystem/FileContext.cpp @@ -3,9 +3,13 @@ #include #include +#include -FileContext::FileContext( - std::filesystem::path path, std::size_t size, std::size_t chunkSize, std::string openMode, std::size_t offset) +FileContext::FileContext(const std::filesystem::path &path, + std::size_t size, + std::size_t chunkSize, + const std::string &openMode, + std::size_t offset) : path(path), size(size), offset(offset), chunkSize(chunkSize) { if (!size || !chunkSize) { @@ -29,7 +33,7 @@ FileContext::~FileContext() std::fclose(file); } -FileReadContext::FileReadContext(std::filesystem::path path, +FileReadContext::FileReadContext(const std::filesystem::path &path, std::size_t size, std::size_t chunkSize, std::size_t offset) @@ -39,9 +43,12 @@ FileReadContext::FileReadContext(std::filesystem::path path, FileReadContext::~FileReadContext() {} -FileWriteContext::FileWriteContext( - std::filesystem::path path, std::size_t size, std::size_t chunkSize, std::string crc32Digest, std::size_t offset) - : FileContext(path, size, chunkSize, "wb", offset), crc32Digest(crc32Digest) +FileWriteContext::FileWriteContext(const std::filesystem::path &path, + std::size_t size, + std::size_t chunkSize, + std::string crc32Digest, + std::size_t offset) + : FileContext(path, size, chunkSize, "wb", offset), crc32Digest(std::move(crc32Digest)) {} FileWriteContext::~FileWriteContext() @@ -77,34 +84,9 @@ auto FileContext::validateChunkRequest(std::uint32_t chunkNo) const -> bool return !(chunkNo < 1 || chunkNo > totalChunksInFile() || chunkNo != expectedChunkInFile()); } -auto FileReadContext::getFileHash() -> std::string +auto FileContext::fileHash() const -> std::string { - std::vector buffer(chunkSize); - - auto dataSize = size; - - while (dataSize) { - - auto dataLeft = std::min(static_cast(chunkSize), dataSize); - auto dataRead = std::fread(buffer.data(), sizeof(int8_t), dataLeft, file); - - if (dataRead != dataLeft) { - LOG_ERROR("File %s read error", path.c_str()); - throw std::runtime_error("File read error"); - } - - runningCrc32Digest.add(buffer.data(), dataRead); - - dataSize -= dataRead; - } - - std::fseek(file, 0, SEEK_SET); - - auto fileHash = runningCrc32Digest.getHash(); - - runningCrc32Digest.reset(); - - return fileHash; + return runningCrc32Digest.getHash(); } auto FileReadContext::read() -> std::vector @@ -124,8 +106,9 @@ auto FileReadContext::read() -> std::vector throw std::runtime_error("File read error"); } - LOG_DEBUG("Read %u bytes", static_cast(dataRead)); + runningCrc32Digest.add(buffer.data(), dataRead); + LOG_DEBUG("Read %u bytes", static_cast(dataRead)); advanceFileOffset(dataRead); if (reachedEOF()) { @@ -159,19 +142,12 @@ auto FileWriteContext::write(const std::vector &data) -> void if (reachedEOF()) { LOG_INFO("Reached EOF of %s", path.c_str()); } - - return; -} - -auto FileWriteContext::fileHash() const -> std::string -{ - return runningCrc32Digest.getHash(); } auto FileWriteContext::crc32Matches() const -> bool { LOG_DEBUG("Hash: %s", fileHash().c_str()); - return crc32Digest.compare(fileHash()) == 0; + return crc32Digest == fileHash(); } auto FileWriteContext::removeFile() -> void diff --git a/module-services/service-desktop/endpoints/filesystem/FileOperations.cpp b/module-services/service-desktop/endpoints/filesystem/FileOperations.cpp index 4b035858f08c1940b9185e1df0c52c6ae7f0e7df..e21a53aed56052f3456e946d2e85c61cacb79ed2 100644 --- a/module-services/service-desktop/endpoints/filesystem/FileOperations.cpp +++ b/module-services/service-desktop/endpoints/filesystem/FileOperations.cpp @@ -75,7 +75,7 @@ auto FileOperations::createFileReadContextFor(const std::filesystem::path &file, auto FileOperations::createFileWriteContextFor(const std::filesystem::path &file, std::size_t fileSize, - const std::string Crc32, + const std::string &Crc32, transfer_id xfrId) -> void { writeTransfers.insert( @@ -120,32 +120,12 @@ auto FileOperations::decodeDataFromBase64(const std::string &encodedData) const return decodedData; } -auto FileOperations::getFileHashForReceiveID(transfer_id rxID) -> std::string -{ - LOG_DEBUG("Getting hash for rxID %u", static_cast(rxID)); - - const auto fileCtxEntry = readTransfers.find(rxID); - - if (fileCtxEntry == readTransfers.end()) { - LOG_ERROR("Invalid rxID %u", static_cast(rxID)); - return {}; - } - - auto fileCtx = fileCtxEntry->second.get(); - - if (!fileCtx) { - LOG_ERROR("Invalid fileCtx for rxID %u", static_cast(rxID)); - return {}; - } - - return fileCtx->getFileHash(); -} - -auto FileOperations::getDataForReceiveID(transfer_id rxID, std::uint32_t chunkNo) -> std::string +auto FileOperations::getDataForReceiveID(transfer_id rxID, std::uint32_t chunkNo) -> DataWithCrc32 { LOG_DEBUG("Getting chunk %u for rxID %u", static_cast(chunkNo), static_cast(rxID)); const auto fileCtxEntry = readTransfers.find(rxID); + std::string fileCrc32 = {}; if (fileCtxEntry == readTransfers.end()) { LOG_ERROR("Invalid rxID %u", static_cast(rxID)); @@ -173,16 +153,16 @@ auto FileOperations::getDataForReceiveID(transfer_id rxID, std::uint32_t chunkNo if (fileCtx->reachedEOF()) { LOG_INFO("Reached EOF for rxID %u", static_cast(rxID)); - + fileCrc32 = fileCtx->fileHash(); writeTransfers.erase(rxID); } - return encodeDataAsBase64(data); + return {std::move(encodeDataAsBase64(data)), std::move(fileCrc32)}; } auto FileOperations::createTransmitIDForFile(const std::filesystem::path &file, std::size_t size, - const std::string Crc32) -> transfer_id + const std::string &Crc32) -> transfer_id { cancelTimedOutWriteTransfer(); const auto txID = ++runningXfrId; diff --git a/module-services/service-desktop/endpoints/filesystem/FilesystemEndpoint.cpp b/module-services/service-desktop/endpoints/filesystem/FilesystemEndpoint.cpp index b4828a218829ea33c5e65b8740d5528a266701cc..f0ae7caeab732e840cefa4175aceff881ab63ba1 100644 --- a/module-services/service-desktop/endpoints/filesystem/FilesystemEndpoint.cpp +++ b/module-services/service-desktop/endpoints/filesystem/FilesystemEndpoint.cpp @@ -75,13 +75,10 @@ namespace sdesktop::endpoints try { auto [rxID, fileSize] = fileOps.createReceiveIDForFile(filePath); - auto fileHash = fileOps.getFileHashForReceiveID(rxID); - context.setResponseStatus(http::Code::OK); context.setResponseBody( json11::Json::object({{json::filesystem::rxID, static_cast(rxID)}, {json::fileSize, static_cast(fileSize)}, - {json::fileCrc32, fileHash}, {json::filesystem::chunkSize, static_cast(FileOperations::ChunkSize)}})); } catch (std::runtime_error &e) { @@ -107,10 +104,10 @@ namespace sdesktop::endpoints auto returnCode = sys::ReturnCodes::Failure; const auto rxID = context.getBody()[json::filesystem::rxID].int_value(); const auto chunkNo = context.getBody()[json::filesystem::chunkNo].int_value(); - std::string data{}; + FileOperations::DataWithCrc32 dataWithCrc32; try { - data = fileOps.getDataForReceiveID(rxID, chunkNo); + dataWithCrc32 = fileOps.getDataForReceiveID(rxID, chunkNo); } catch (std::exception &e) { LOG_ERROR("%s", e.what()); @@ -120,11 +117,17 @@ namespace sdesktop::endpoints return sys::ReturnCodes::Failure; } - if (data.length()) { + if (dataWithCrc32.data.length()) { context.setResponseStatus(http::Code::OK); - context.setResponseBody(json11::Json::object({{json::filesystem::rxID, static_cast(rxID)}, - {json::filesystem::chunkNo, static_cast(chunkNo)}, - {json::filesystem::data, data}})); + json11::Json::object contextJsonObject = + json11::Json::object({{json::filesystem::rxID, static_cast(rxID)}, + {json::filesystem::chunkNo, static_cast(chunkNo)}, + {json::filesystem::data, dataWithCrc32.data}}); + + if (dataWithCrc32.crc32.length()) { + contextJsonObject[json::fileCrc32] = dataWithCrc32.crc32; + } + context.setResponseBody(contextJsonObject); returnCode = sys::ReturnCodes::Success; } diff --git a/module-services/service-desktop/endpoints/include/endpoints/filesystem/FileContext.hpp b/module-services/service-desktop/endpoints/include/endpoints/filesystem/FileContext.hpp index e2e65d03ea2e7349176cce7972db489a2ced4095..0fa36f5ed27217b7c973758deb7abb8bba205748 100644 --- a/module-services/service-desktop/endpoints/include/endpoints/filesystem/FileContext.hpp +++ b/module-services/service-desktop/endpoints/include/endpoints/filesystem/FileContext.hpp @@ -16,10 +16,10 @@ class FileContext { public: - explicit FileContext(std::filesystem::path path, + explicit FileContext(const std::filesystem::path &path, std::size_t size, std::size_t chunkSize, - std::string openMode, + const std::string &openMode, std::size_t offset = 0); virtual ~FileContext(); @@ -36,6 +36,8 @@ class FileContext auto expectedChunkInFile() const -> std::size_t; + auto fileHash() const -> std::string; + protected: std::filesystem::path path{}; std::FILE *file{}; @@ -49,22 +51,20 @@ class FileContext class FileReadContext : public FileContext { public: - explicit FileReadContext(std::filesystem::path path, + explicit FileReadContext(const std::filesystem::path &path, std::size_t size, std::size_t chunkSize, std::size_t offset = 0); ~FileReadContext(); - auto getFileHash() -> std::string; - auto read() -> std::vector; }; class FileWriteContext : public FileContext { public: - explicit FileWriteContext(std::filesystem::path path, + explicit FileWriteContext(const std::filesystem::path &path, std::size_t size, std::size_t chunkSize, std::string crc32Digest, @@ -72,8 +72,6 @@ class FileWriteContext : public FileContext ~FileWriteContext(); - auto fileHash() const -> std::string; - auto crc32Matches() const -> bool; auto removeFile() -> void; diff --git a/module-services/service-desktop/endpoints/include/endpoints/filesystem/FileOperations.hpp b/module-services/service-desktop/endpoints/include/endpoints/filesystem/FileOperations.hpp index 7a095e90659a7c540bd0765588cc8de06c258514..8c8c4c43025136a7537255f11a825b1f1f3be213 100644 --- a/module-services/service-desktop/endpoints/include/endpoints/filesystem/FileOperations.hpp +++ b/module-services/service-desktop/endpoints/include/endpoints/filesystem/FileOperations.hpp @@ -29,7 +29,7 @@ class FileOperations auto createFileWriteContextFor(const std::filesystem::path &file, std::size_t fileSize, - const std::string Crc32, + const std::string &Crc32, transfer_id xfrId) -> void; auto encodeDataAsBase64(const std::vector &binaryData) const -> std::string; @@ -53,15 +53,19 @@ class FileOperations static constexpr auto ChunkSizeMultiplier = 12u; static constexpr auto ChunkSize = ChunkSizeMultiplier * SingleChunkSize; + struct DataWithCrc32 + { + std::string data; + std::string crc32; + }; + static FileOperations &instance(); auto createReceiveIDForFile(const std::filesystem::path &file) -> std::pair; - auto getDataForReceiveID(transfer_id, std::uint32_t chunkNo) -> std::string; - - auto getFileHashForReceiveID(transfer_id rxID) -> std::string; + auto getDataForReceiveID(transfer_id, std::uint32_t chunkNo) -> DataWithCrc32; - auto createTransmitIDForFile(const std::filesystem::path &file, std::size_t size, const std::string Crc32) + auto createTransmitIDForFile(const std::filesystem::path &file, std::size_t size, const std::string &Crc32) -> transfer_id; auto sendDataForTransmitID(transfer_id, std::uint32_t chunkNo, const std::string &data) -> sys::ReturnCodes; diff --git a/test/harness b/test/harness index 109327d1a7a73afad9f741e3c17d1e161e8a9c01..8a33fa2634c6e24b9099e23404ab5104678ac426 160000 --- a/test/harness +++ b/test/harness @@ -1 +1 @@ -Subproject commit 109327d1a7a73afad9f741e3c17d1e161e8a9c01 +Subproject commit 8a33fa2634c6e24b9099e23404ab5104678ac426