From ecb217280fefcf2a4306f39245c881ad38a96f16 Mon Sep 17 00:00:00 2001 From: Alex Zolotarev Date: Thu, 10 Nov 2011 12:23:26 +0300 Subject: [PATCH] [downloader] Some fixes after code review --- platform/chunks_download_strategy.cpp | 16 ++--- platform/chunks_download_strategy.hpp | 9 +-- platform/http_request.cpp | 55 +++++++------- platform/http_request.hpp | 8 +-- platform/platform_tests/downloader_test.cpp | 80 ++++++++++----------- 5 files changed, 84 insertions(+), 84 deletions(-) diff --git a/platform/chunks_download_strategy.cpp b/platform/chunks_download_strategy.cpp index 88ead660a8..bc9fe13fdd 100644 --- a/platform/chunks_download_strategy.cpp +++ b/platform/chunks_download_strategy.cpp @@ -8,7 +8,7 @@ namespace downloader { -ChunksDownloadStrategy::RangeT const ChunksDownloadStrategy::INVALID_RANGE = RangeT(-1, -1); +ChunksDownloadStrategy::RangeT const ChunksDownloadStrategy::INVALID_RANGE = RangeT(INVALID_CHUNK, INVALID_CHUNK); ChunksDownloadStrategy::ChunksDownloadStrategy(vector const & urls, int64_t fileSize, int64_t chunkSize) @@ -28,21 +28,21 @@ void ChunksDownloadStrategy::SetChunksToDownload(RangesContainerT & chunks) m_chunksToDownload.swap(chunks); } -void ChunksDownloadStrategy::ChunkFinished(bool successfully, int64_t begRange, int64_t endRange) +void ChunksDownloadStrategy::ChunkFinished(bool successfully, RangeT const & range) { - RangeT const chunk(begRange, endRange); // find server which was downloading this chunk for (ServersT::iterator it = m_servers.begin(); it != m_servers.end(); ++it) { - if (it->second == chunk) + if (it->second == range) { if (successfully) it->second = INVALID_RANGE; else { + // @TODO implement connection retry // remove failed server and mark chunk as not downloaded m_servers.erase(it); - m_chunksToDownload.insert(chunk); + m_chunksToDownload.insert(range); } break; } @@ -50,8 +50,7 @@ void ChunksDownloadStrategy::ChunkFinished(bool successfully, int64_t begRange, } ChunksDownloadStrategy::ResultT ChunksDownloadStrategy::NextChunk(string & outUrl, - int64_t & begRange, - int64_t & endRange) + RangeT & range) { if (m_servers.empty()) return EDownloadFailed; @@ -80,8 +79,7 @@ ChunksDownloadStrategy::ResultT ChunksDownloadStrategy::NextChunk(string & outUr // found not used server m_servers[i].second = nextChunk; outUrl = m_servers[i].first; - begRange = nextChunk.first; - endRange = nextChunk.second; + range = nextChunk; m_chunksToDownload.erase(m_chunksToDownload.begin()); return ENextChunk; } diff --git a/platform/chunks_download_strategy.hpp b/platform/chunks_download_strategy.hpp index 44442f4738..5ac930a9ec 100644 --- a/platform/chunks_download_strategy.hpp +++ b/platform/chunks_download_strategy.hpp @@ -8,6 +8,7 @@ namespace downloader { +/// Single-threaded code class ChunksDownloadStrategy { public: @@ -24,11 +25,11 @@ private: public: /// @param[in] chunksToDownload used for resume - ChunksDownloadStrategy(vector const & urls, int64_t fileSize, int64_t chunkSize = 512 * 1024); + ChunksDownloadStrategy(vector const & urls, int64_t fileSize, int64_t chunkSize); int64_t ChunkSize() const { return m_chunkSize; } /// Should be called when each chunk is completed - void ChunkFinished(bool successfully, int64_t begRange, int64_t endRange); + void ChunkFinished(bool successfully, RangeT const & range); enum ResultT { ENextChunk, @@ -37,8 +38,8 @@ public: EDownloadSucceeded }; /// Should be called until returns ENextChunk - ResultT NextChunk(string & outUrl, int64_t & begRange, int64_t & endRange); - + ResultT NextChunk(string & outUrl, RangeT & range); + /// Used for resume support - external code knows when it's necessary void SetChunksToDownload(RangesContainerT & chunks); RangesContainerT const & ChunksLeft() const { return m_chunksToDownload; } }; diff --git a/platform/http_request.cpp b/platform/http_request.cpp index b1af6b8a80..30de25f693 100644 --- a/platform/http_request.cpp +++ b/platform/http_request.cpp @@ -25,19 +25,10 @@ HttpThread * CreateNativeHttpThread(string const & url, int64_t endRange = -1, int64_t expectedSize = -1, string const & postBody = string()); -void DeleteNativeHttpThread(HttpThread * request); +void DeleteNativeHttpThread(HttpThread * thread); ////////////////////////////////////////////////////////////////////////////////////////// -HttpRequest::HttpRequest(CallbackT onFinish, CallbackT onProgress) - : m_status(EInProgress), m_progress(make_pair(0, -1)), - m_onFinish(onFinish), m_onProgress(onProgress) -{ -} - -HttpRequest::~HttpRequest() -{ -} -////////////////////////////////////////////////////////////////////////////////////////////////////////// +/// Stores server response into the memory class MemoryHttpRequest : public HttpRequest, public IHttpThreadCallback { HttpThread * m_thread; @@ -84,17 +75,6 @@ public: } }; -HttpRequest * HttpRequest::Get(string const & url, CallbackT onFinish, CallbackT onProgress) -{ - return new MemoryHttpRequest(url, onFinish, onProgress); -} - -HttpRequest * HttpRequest::PostJson(string const & url, string const & postData, - CallbackT onFinish, CallbackT onProgress) -{ - return new MemoryHttpRequest(url, postData, onFinish, onProgress); -} - //////////////////////////////////////////////////////////////////////////////////////////////// class FileHttpRequest : public HttpRequest, public IHttpThreadCallback { @@ -111,10 +91,11 @@ class FileHttpRequest : public HttpRequest, public IHttpThreadCallback ChunksDownloadStrategy::ResultT StartThreads() { string url; - int64_t beg, end; + ChunksDownloadStrategy::RangeT range; ChunksDownloadStrategy::ResultT result; - while ((result = m_strategy.NextChunk(url, beg, end)) == ChunksDownloadStrategy::ENextChunk) - m_threads.push_back(make_pair(CreateNativeHttpThread(url, *this, beg, end, m_progress.second), beg)); + while ((result = m_strategy.NextChunk(url, range)) == ChunksDownloadStrategy::ENextChunk) + m_threads.push_back(make_pair(CreateNativeHttpThread(url, *this, range.first, range.second, + m_progress.second), range.first)); return result; } @@ -146,7 +127,7 @@ class FileHttpRequest : public HttpRequest, public IHttpThreadCallback static threads::ThreadID const id = threads::GetCurrentThreadID(); ASSERT_EQUAL(id, threads::GetCurrentThreadID(), ("OnFinish called from different threads")); #endif - m_strategy.ChunkFinished(httpCode == 200, begRange, endRange); + m_strategy.ChunkFinished(httpCode == 200, ChunksDownloadStrategy::RangeT(begRange, endRange)); // remove completed chunk from the list, beg is the key RemoveHttpThreadByKey(begRange); @@ -252,6 +233,28 @@ public: } }; +////////////////////////////////////////////////////////////////////////////////////////////////////////// +HttpRequest::HttpRequest(CallbackT onFinish, CallbackT onProgress) + : m_status(EInProgress), m_progress(make_pair(0, -1)), + m_onFinish(onFinish), m_onProgress(onProgress) +{ +} + +HttpRequest::~HttpRequest() +{ +} + +HttpRequest * HttpRequest::Get(string const & url, CallbackT onFinish, CallbackT onProgress) +{ + return new MemoryHttpRequest(url, onFinish, onProgress); +} + +HttpRequest * HttpRequest::PostJson(string const & url, string const & postData, + CallbackT onFinish, CallbackT onProgress) +{ + return new MemoryHttpRequest(url, postData, onFinish, onProgress); +} + HttpRequest * HttpRequest::GetFile(vector const & urls, string const & filePath, int64_t fileSize, CallbackT onFinish, CallbackT onProgress, int64_t chunkSize) { diff --git a/platform/http_request.hpp b/platform/http_request.hpp index 8323911de3..e0f5c28d79 100644 --- a/platform/http_request.hpp +++ b/platform/http_request.hpp @@ -44,11 +44,11 @@ public: CallbackT onProgress = CallbackT()); /// Content-type for request is always "application/json" static HttpRequest * PostJson(string const & url, string const & postData, - CallbackT onFinish, CallbackT onProgress = CallbackT()); + CallbackT onFinish, CallbackT onProgress = CallbackT()); static HttpRequest * GetFile(vector const & urls, string const & filePath, - int64_t projectedFileSize, - CallbackT onFinish, CallbackT onProgress = CallbackT(), - int64_t chunkSize = 512 * 1024); + int64_t projectedFileSize, + CallbackT onFinish, CallbackT onProgress = CallbackT(), + int64_t chunkSize = 512 * 1024); }; } // namespace downloader diff --git a/platform/platform_tests/downloader_test.cpp b/platform/platform_tests/downloader_test.cpp index 8cc2d32b11..2e56d401fb 100644 --- a/platform/platform_tests/downloader_test.cpp +++ b/platform/platform_tests/downloader_test.cpp @@ -188,7 +188,7 @@ UNIT_TEST(ChunksDownloadStrategy) string const S1 = "UrlOfServer1"; string const S2 = "UrlOfServer2"; string const S3 = "UrlOfServer3"; - pair const R1(0, 249), R2(250, 499), R3(500, 749), R4(750, 800); + ChunksDownloadStrategy::RangeT const R1(0, 249), R2(250, 499), R3(500, 749), R4(750, 800); vector servers; servers.push_back(S1); servers.push_back(S2); @@ -198,74 +198,72 @@ UNIT_TEST(ChunksDownloadStrategy) ChunksDownloadStrategy strategy(servers, FILE_SIZE, CHUNK_SIZE); string s1; - int64_t beg1, end1; - TEST_EQUAL(strategy.NextChunk(s1, beg1, end1), ChunksDownloadStrategy::ENextChunk, ()); + ChunksDownloadStrategy::RangeT r1; + TEST_EQUAL(strategy.NextChunk(s1, r1), ChunksDownloadStrategy::ENextChunk, ()); string s2; - int64_t beg2, end2; - TEST_EQUAL(strategy.NextChunk(s2, beg2, end2), ChunksDownloadStrategy::ENextChunk, ()); + ChunksDownloadStrategy::RangeT r2; + TEST_EQUAL(strategy.NextChunk(s2, r2), ChunksDownloadStrategy::ENextChunk, ()); string s3; - int64_t beg3, end3; - TEST_EQUAL(strategy.NextChunk(s3, beg3, end3), ChunksDownloadStrategy::ENextChunk, ()); + ChunksDownloadStrategy::RangeT r3; + TEST_EQUAL(strategy.NextChunk(s3, r3), ChunksDownloadStrategy::ENextChunk, ()); string sEmpty; - int64_t begEmpty, endEmpty; - TEST_EQUAL(strategy.NextChunk(sEmpty, begEmpty, endEmpty), ChunksDownloadStrategy::ENoFreeServers, ()); - TEST_EQUAL(strategy.NextChunk(sEmpty, begEmpty, endEmpty), ChunksDownloadStrategy::ENoFreeServers, ()); + ChunksDownloadStrategy::RangeT rEmpty; + TEST_EQUAL(strategy.NextChunk(sEmpty, rEmpty), ChunksDownloadStrategy::ENoFreeServers, ()); + TEST_EQUAL(strategy.NextChunk(sEmpty, rEmpty), ChunksDownloadStrategy::ENoFreeServers, ()); TEST(s1 != s2 && s2 != s3 && s3 != s1, (s1, s2, s3)); - pair const r1(beg1, end1), r2(beg2, end2), r3(beg3, end3); TEST(r1 != r2 && r2 != r3 && r3 != r1, (r1, r2, r3)); TEST(r1 == R1 || r1 == R2 || r1 == R3 || r1 == R4, (r1)); TEST(r2 == R1 || r2 == R2 || r2 == R3 || r2 == R4, (r2)); TEST(r3 == R1 || r3 == R2 || r3 == R3 || r3 == R4, (r3)); - strategy.ChunkFinished(true, beg1, end1); + strategy.ChunkFinished(true, r1); string s4; - int64_t beg4, end4; - TEST_EQUAL(strategy.NextChunk(s4, beg4, end4), ChunksDownloadStrategy::ENextChunk, ()); + ChunksDownloadStrategy::RangeT r4; + TEST_EQUAL(strategy.NextChunk(s4, r4), ChunksDownloadStrategy::ENextChunk, ()); TEST_EQUAL(s4, s1, ()); - pair const r4(beg4, end4); TEST(r4 != r1 && r4 != r2 && r4 != r3, (r4)); - TEST_EQUAL(strategy.NextChunk(sEmpty, begEmpty, endEmpty), ChunksDownloadStrategy::ENoFreeServers, ()); - TEST_EQUAL(strategy.NextChunk(sEmpty, begEmpty, endEmpty), ChunksDownloadStrategy::ENoFreeServers, ()); + TEST_EQUAL(strategy.NextChunk(sEmpty, rEmpty), ChunksDownloadStrategy::ENoFreeServers, ()); + TEST_EQUAL(strategy.NextChunk(sEmpty, rEmpty), ChunksDownloadStrategy::ENoFreeServers, ()); - strategy.ChunkFinished(false, beg2, end2); + strategy.ChunkFinished(false, r2); - TEST_EQUAL(strategy.NextChunk(sEmpty, begEmpty, endEmpty), ChunksDownloadStrategy::ENoFreeServers, ()); + TEST_EQUAL(strategy.NextChunk(sEmpty, rEmpty), ChunksDownloadStrategy::ENoFreeServers, ()); - strategy.ChunkFinished(true, beg4, end4); + strategy.ChunkFinished(true, r4); string s5; - int64_t beg5, end5; - TEST_EQUAL(strategy.NextChunk(s5, beg5, end5), ChunksDownloadStrategy::ENextChunk, ()); + ChunksDownloadStrategy::RangeT r5; + TEST_EQUAL(strategy.NextChunk(s5, r5), ChunksDownloadStrategy::ENextChunk, ()); TEST_EQUAL(s5, s4, (s5, s4)); - TEST(beg5 == beg2 && end5 == end2, ()); + TEST_EQUAL(r5, r2, ()); - TEST_EQUAL(strategy.NextChunk(sEmpty, begEmpty, endEmpty), ChunksDownloadStrategy::ENoFreeServers, ()); - TEST_EQUAL(strategy.NextChunk(sEmpty, begEmpty, endEmpty), ChunksDownloadStrategy::ENoFreeServers, ()); + TEST_EQUAL(strategy.NextChunk(sEmpty, rEmpty), ChunksDownloadStrategy::ENoFreeServers, ()); + TEST_EQUAL(strategy.NextChunk(sEmpty, rEmpty), ChunksDownloadStrategy::ENoFreeServers, ()); - strategy.ChunkFinished(true, beg5, end5); + strategy.ChunkFinished(true, r5); // 3rd is still alive here - TEST_EQUAL(strategy.NextChunk(sEmpty, begEmpty, endEmpty), ChunksDownloadStrategy::ENoFreeServers, ()); - TEST_EQUAL(strategy.NextChunk(sEmpty, begEmpty, endEmpty), ChunksDownloadStrategy::ENoFreeServers, ()); + TEST_EQUAL(strategy.NextChunk(sEmpty, rEmpty), ChunksDownloadStrategy::ENoFreeServers, ()); + TEST_EQUAL(strategy.NextChunk(sEmpty, rEmpty), ChunksDownloadStrategy::ENoFreeServers, ()); - strategy.ChunkFinished(true, beg3, end3); + strategy.ChunkFinished(true, r3); - TEST_EQUAL(strategy.NextChunk(sEmpty, begEmpty, endEmpty), ChunksDownloadStrategy::EDownloadSucceeded, ()); - TEST_EQUAL(strategy.NextChunk(sEmpty, begEmpty, endEmpty), ChunksDownloadStrategy::EDownloadSucceeded, ()); + TEST_EQUAL(strategy.NextChunk(sEmpty, rEmpty), ChunksDownloadStrategy::EDownloadSucceeded, ()); + TEST_EQUAL(strategy.NextChunk(sEmpty, rEmpty), ChunksDownloadStrategy::EDownloadSucceeded, ()); } UNIT_TEST(ChunksDownloadStrategyFAIL) { string const S1 = "UrlOfServer1"; string const S2 = "UrlOfServer2"; - pair const R1(0, 249), R2(250, 499), R3(500, 749), R4(750, 800); + ChunksDownloadStrategy::RangeT const R1(0, 249), R2(250, 499), R3(500, 749), R4(750, 800); vector servers; servers.push_back(S1); servers.push_back(S2); @@ -274,20 +272,20 @@ UNIT_TEST(ChunksDownloadStrategyFAIL) ChunksDownloadStrategy strategy(servers, FILE_SIZE, CHUNK_SIZE); string s1; - int64_t beg1, end1; - TEST_EQUAL(strategy.NextChunk(s1, beg1, end1), ChunksDownloadStrategy::ENextChunk, ()); + ChunksDownloadStrategy::RangeT r1; + TEST_EQUAL(strategy.NextChunk(s1, r1), ChunksDownloadStrategy::ENextChunk, ()); string s2; - int64_t beg2, end2; - TEST_EQUAL(strategy.NextChunk(s2, beg2, end2), ChunksDownloadStrategy::ENextChunk, ()); - TEST_EQUAL(strategy.NextChunk(s2, beg2, end2), ChunksDownloadStrategy::ENoFreeServers, ()); + ChunksDownloadStrategy::RangeT r2; + TEST_EQUAL(strategy.NextChunk(s2, r2), ChunksDownloadStrategy::ENextChunk, ()); + TEST_EQUAL(strategy.NextChunk(s2, r2), ChunksDownloadStrategy::ENoFreeServers, ()); - strategy.ChunkFinished(false, beg1, end1); + strategy.ChunkFinished(false, r1); - TEST_EQUAL(strategy.NextChunk(s2, beg2, end2), ChunksDownloadStrategy::ENoFreeServers, ()); + TEST_EQUAL(strategy.NextChunk(s2, r2), ChunksDownloadStrategy::ENoFreeServers, ()); - strategy.ChunkFinished(false, beg2, end2); + strategy.ChunkFinished(false, r2); - TEST_EQUAL(strategy.NextChunk(s2, beg2, end2), ChunksDownloadStrategy::EDownloadFailed, ()); + TEST_EQUAL(strategy.NextChunk(s2, r2), ChunksDownloadStrategy::EDownloadFailed, ()); } bool ReadFileAsString(string const & file, string & outStr)