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 >**_
This commit is contained in:
committed by
GitHub
parent
6ff5fcd9a7
commit
45a228a645
@@ -9,10 +9,49 @@
|
||||
|
||||
#include <cstring>
|
||||
#include <memory>
|
||||
#include <utility>
|
||||
|
||||
#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<NetworkClient> 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<size_t>(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);
|
||||
|
||||
@@ -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;
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user