From 45a228a64594055e1f4b44937b1323c4d7d9f2a3 Mon Sep 17 00:00:00 2001 From: Arthur Tazhitdinov Date: Sat, 28 Feb 2026 13:39:09 +0300 Subject: [PATCH] fix: use HTTPClient::writeToStream for downloading files from OPDS (#1207) ## Summary * Refactored `HttpDownloader::downloadToFile` to use `FileWriteStream` and `HTTPClient::writeToStream`, removing manual chunked downloading logic, which was error-prone. * Fixes https://github.com/crosspoint-reader/crosspoint-reader/issues/632 ## Additional Context * Tested downloading files from OPDS with a size up to 10 mb. --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? _**< YES >**_ --- .../browser/OpdsBookBrowserActivity.cpp | 5 +- src/network/HttpDownloader.cpp | 115 +++++++++++------- src/network/HttpDownloader.h | 3 - 3 files changed, 73 insertions(+), 50 deletions(-) diff --git a/src/activities/browser/OpdsBookBrowserActivity.cpp b/src/activities/browser/OpdsBookBrowserActivity.cpp index 5b43da4c..f6a58385 100644 --- a/src/activities/browser/OpdsBookBrowserActivity.cpp +++ b/src/activities/browser/OpdsBookBrowserActivity.cpp @@ -171,7 +171,10 @@ void OpdsBookBrowserActivity::render(RenderLock&&) { if (state == BrowserState::DOWNLOADING) { renderer.drawCenteredText(UI_10_FONT_ID, pageHeight / 2 - 40, tr(STR_DOWNLOADING)); - renderer.drawCenteredText(UI_10_FONT_ID, pageHeight / 2 - 10, statusMessage.c_str()); + const auto maxWidth = pageWidth - 40; + // Trim long titles to keep them within the screen bounds. + auto title = renderer.truncatedText(UI_10_FONT_ID, statusMessage.c_str(), maxWidth); + renderer.drawCenteredText(UI_10_FONT_ID, pageHeight / 2 - 10, title.c_str()); if (downloadTotal > 0) { const int barWidth = pageWidth - 100; constexpr int barHeight = 20; diff --git a/src/network/HttpDownloader.cpp b/src/network/HttpDownloader.cpp index 0335e929..799b8874 100644 --- a/src/network/HttpDownloader.cpp +++ b/src/network/HttpDownloader.cpp @@ -9,10 +9,49 @@ #include #include +#include #include "CrossPointSettings.h" #include "util/UrlUtils.h" +namespace { +class FileWriteStream final : public Stream { + public: + FileWriteStream(FsFile& file, size_t total, HttpDownloader::ProgressCallback progress) + : file_(file), total_(total), progress_(std::move(progress)) {} + + size_t write(uint8_t byte) override { return write(&byte, 1); } + + size_t write(const uint8_t* buffer, size_t size) override { + // Write-through stream for HTTPClient::writeToStream with progress tracking. + const size_t written = file_.write(buffer, size); + if (written != size) { + writeOk_ = false; + } + downloaded_ += written; + if (progress_ && total_ > 0) { + progress_(downloaded_, total_); + } + return written; + } + + int available() override { return 0; } + int read() override { return -1; } + int peek() override { return -1; } + void flush() override { file_.flush(); } + + size_t downloaded() const { return downloaded_; } + bool ok() const { return writeOk_; } + + private: + FsFile& file_; + size_t total_; + size_t downloaded_ = 0; + bool writeOk_ = true; + HttpDownloader::ProgressCallback progress_; +}; +} // namespace + bool HttpDownloader::fetchUrl(const std::string& url, Stream& outContent) { // Use NetworkClientSecure for HTTPS, regular NetworkClient for HTTP std::unique_ptr client; @@ -96,8 +135,13 @@ HttpDownloader::DownloadError HttpDownloader::downloadToFile(const std::string& return HTTP_ERROR; } - const size_t contentLength = http.getSize(); - LOG_DBG("HTTP", "Content-Length: %zu", contentLength); + const int64_t reportedLength = http.getSize(); + const size_t contentLength = reportedLength > 0 ? static_cast(reportedLength) : 0; + if (contentLength > 0) { + LOG_DBG("HTTP", "Content-Length: %zu", contentLength); + } else { + LOG_DBG("HTTP", "Content-Length: unknown"); + } // Remove existing file if present if (Storage.exists(destPath.c_str())) { @@ -112,56 +156,35 @@ HttpDownloader::DownloadError HttpDownloader::downloadToFile(const std::string& return FILE_ERROR; } - // Get the stream for chunked reading - NetworkClient* stream = http.getStreamPtr(); - if (!stream) { - LOG_ERR("HTTP", "Failed to get stream"); - file.close(); - Storage.remove(destPath.c_str()); - http.end(); - return HTTP_ERROR; - } - - // Download in chunks - uint8_t buffer[DOWNLOAD_CHUNK_SIZE]; - size_t downloaded = 0; - const size_t total = contentLength > 0 ? contentLength : 0; - - while (http.connected() && (contentLength == 0 || downloaded < contentLength)) { - const size_t available = stream->available(); - if (available == 0) { - delay(1); - continue; - } - - const size_t toRead = available < DOWNLOAD_CHUNK_SIZE ? available : DOWNLOAD_CHUNK_SIZE; - const size_t bytesRead = stream->readBytes(buffer, toRead); - - if (bytesRead == 0) { - break; - } - - const size_t written = file.write(buffer, bytesRead); - if (written != bytesRead) { - LOG_ERR("HTTP", "Write failed: wrote %zu of %zu bytes", written, bytesRead); - file.close(); - Storage.remove(destPath.c_str()); - http.end(); - return FILE_ERROR; - } - - downloaded += bytesRead; - - if (progress && total > 0) { - progress(downloaded, total); - } - } + // Let HTTPClient handle chunked decoding and stream body bytes into the file. + FileWriteStream fileStream(file, contentLength, progress); + const int writeResult = http.writeToStream(&fileStream); file.close(); http.end(); + if (writeResult < 0) { + LOG_ERR("HTTP", "writeToStream error: %d", writeResult); + Storage.remove(destPath.c_str()); + return HTTP_ERROR; + } + + const size_t downloaded = fileStream.downloaded(); LOG_DBG("HTTP", "Downloaded %zu bytes", downloaded); + // Guard against partial writes even if HTTPClient completes. + if (!fileStream.ok()) { + LOG_ERR("HTTP", "Write failed during download"); + Storage.remove(destPath.c_str()); + return FILE_ERROR; + } + + if (contentLength == 0 && downloaded == 0) { + LOG_ERR("HTTP", "Download failed: no data received"); + Storage.remove(destPath.c_str()); + return HTTP_ERROR; + } + // Verify download size if known if (contentLength > 0 && downloaded != contentLength) { LOG_ERR("HTTP", "Size mismatch: got %zu, expected %zu", downloaded, contentLength); diff --git a/src/network/HttpDownloader.h b/src/network/HttpDownloader.h index 5b980242..a8f1cd25 100644 --- a/src/network/HttpDownloader.h +++ b/src/network/HttpDownloader.h @@ -38,7 +38,4 @@ class HttpDownloader { */ static DownloadError downloadToFile(const std::string& url, const std::string& destPath, ProgressCallback progress = nullptr); - - private: - static constexpr size_t DOWNLOAD_CHUNK_SIZE = 1024; };