From 117222c2068aa783263722a65b76f8c95d432143 Mon Sep 17 00:00:00 2001 From: vng Date: Fri, 29 Jun 2012 17:09:24 -0700 Subject: [PATCH] [downloader] Handle exceptions correctly in downloader for FileWriter manipulations. --- .../com/mapswithme/platform/HttpThread.cpp | 14 ++++--- .../maps/downloader/DownloadChunkTask.java | 19 ++++++--- platform/chunks_download_strategy.cpp | 1 - platform/chunks_download_strategy.hpp | 2 + platform/http_request.cpp | 41 +++++++++++++++---- platform/http_thread_callback.hpp | 2 +- storage/storage.cpp | 1 + 7 files changed, 60 insertions(+), 20 deletions(-) diff --git a/android/jni/com/mapswithme/platform/HttpThread.cpp b/android/jni/com/mapswithme/platform/HttpThread.cpp index 66234b32d6..f20af60a67 100644 --- a/android/jni/com/mapswithme/platform/HttpThread.cpp +++ b/android/jni/com/mapswithme/platform/HttpThread.cpp @@ -21,10 +21,10 @@ public: JNIEnv * env = jni::GetEnv(); jclass klass = env->FindClass("com/mapswithme/maps/downloader/DownloadChunkTask"); - ASSERT(klass, ()); + ASSERT ( klass, () ); jmethodID methodId = env->GetMethodID(klass, "", "(JLjava/lang/String;JJJLjava/lang/String;Ljava/lang/String;)V"); - ASSERT(methodId, ()); + ASSERT ( methodId, () ); // User id is always the same, so do not waste time on every chunk call static string uniqueUserId = GetPlatform().UniqueClientId(); @@ -38,9 +38,10 @@ public: (jlong)expectedFileSize, env->NewStringUTF(pb.c_str()), env->NewStringUTF(uniqueUserId.c_str()))); + ASSERT ( m_self, () ); methodId = env->GetMethodID(klass, "start", "()V"); - ASSERT(methodId, ()); + ASSERT ( methodId, () ); env->CallVoidMethod(m_self, methodId); } @@ -76,14 +77,17 @@ namespace downloader extern "C" { - JNIEXPORT void JNICALL + JNIEXPORT jboolean JNICALL Java_com_mapswithme_maps_downloader_DownloadChunkTask_onWrite(JNIEnv * env, jobject thiz, jlong httpCallbackID, jlong beg, jbyteArray data, jlong size) { downloader::IHttpThreadCallback * cb = reinterpret_cast(httpCallbackID); jbyte * buf = env->GetByteArrayElements(data, 0); - cb->OnWrite(beg, buf, size); + ASSERT ( buf, () ); + + bool const ret = cb->OnWrite(beg, buf, size); env->ReleaseByteArrayElements(data, buf, 0); + return ret; } JNIEXPORT void JNICALL diff --git a/android/src/com/mapswithme/maps/downloader/DownloadChunkTask.java b/android/src/com/mapswithme/maps/downloader/DownloadChunkTask.java index 8d9217d142..bcabb83e61 100644 --- a/android/src/com/mapswithme/maps/downloader/DownloadChunkTask.java +++ b/android/src/com/mapswithme/maps/downloader/DownloadChunkTask.java @@ -26,10 +26,11 @@ class DownloadChunkTask extends AsyncTask private final int NOT_SET = -1; private final int IO_ERROR = -2; private final int INVALID_URL = -3; + private final int WRITE_ERROR = -4; private int m_httpErrorCode = NOT_SET; private long m_downloadedBytes = 0; - native void onWrite(long httpCallbackID, long beg, byte [] data, long size); + native boolean onWrite(long httpCallbackID, long beg, byte [] data, long size); native void onFinish(long httpCallbackID, long httpCode, long beg, long end); public DownloadChunkTask(long httpCallbackID, String url, long beg, long end, long expectedFileSize, @@ -63,8 +64,14 @@ class DownloadChunkTask extends AsyncTask if (!isCancelled()) { // Use progress event to save downloaded bytes. - onWrite(m_httpCallbackID, m_beg + m_downloadedBytes, data[0], data[0].length); - m_downloadedBytes += data[0].length; + if (onWrite(m_httpCallbackID, m_beg + m_downloadedBytes, data[0], data[0].length)) + m_downloadedBytes += data[0].length; + else + { + // Cancel downloading and notify about error. + cancel(false); + onFinish(m_httpCallbackID, WRITE_ERROR, m_beg, m_end); + } } } @@ -187,7 +194,7 @@ class DownloadChunkTask extends AsyncTask byte[] tempBuf = new byte[bufferSize]; int readBytes; - while ((readBytes = stream.read(tempBuf)) != -1) + while ((readBytes = stream.read(tempBuf)) > 0) { if (isCancelled()) return false; @@ -198,6 +205,8 @@ class DownloadChunkTask extends AsyncTask publishProgress(chunk); } - return true; + // -1 - means the end of the stream (success). + // 0 - some error occurred. + return (readBytes == -1 ? true : false); } } diff --git a/platform/chunks_download_strategy.cpp b/platform/chunks_download_strategy.cpp index 6e24acb73b..b6cd42f79d 100644 --- a/platform/chunks_download_strategy.cpp +++ b/platform/chunks_download_strategy.cpp @@ -23,7 +23,6 @@ ChunksDownloadStrategy::GetChunk(RangeT const & range) { vector::iterator i = lower_bound(m_chunks.begin(), m_chunks.end(), range.first, LessChunks()); - // find server which was downloading this chunk if (i != m_chunks.end() && i->m_pos == range.first) { ASSERT_EQUAL ( (i+1)->m_pos, range.second + 1, () ); diff --git a/platform/chunks_download_strategy.hpp b/platform/chunks_download_strategy.hpp index d6ab853941..c3ae0cd841 100644 --- a/platform/chunks_download_strategy.hpp +++ b/platform/chunks_download_strategy.hpp @@ -48,6 +48,8 @@ private: }; typedef pair RangeT; + + /// @return Chunk pointer and it's index for given file offsets range. pair GetChunk(RangeT const & range); public: diff --git a/platform/http_request.cpp b/platform/http_request.cpp index e2ded9c01b..a86bf89c61 100644 --- a/platform/http_request.cpp +++ b/platform/http_request.cpp @@ -58,12 +58,13 @@ class MemoryHttpRequest : public HttpRequest, public IHttpThreadCallback string m_downloadedData; MemWriter m_writer; - virtual void OnWrite(int64_t, void const * buffer, size_t size) + virtual bool OnWrite(int64_t, void const * buffer, size_t size) { m_writer.Write(buffer, size); m_progress.first += size; if (m_onProgress) m_onProgress(*this); + return true; } virtual void OnFinish(long httpCode, int64_t, int64_t) @@ -77,6 +78,7 @@ public: : HttpRequest(onFinish, onProgress), m_writer(m_downloadedData) { m_thread = CreateNativeHttpThread(url, *this); + ASSERT ( m_thread, () ); } MemoryHttpRequest(string const & url, string const & postData, @@ -84,6 +86,7 @@ public: : HttpRequest(onFinish, onProgress), m_writer(m_downloadedData) { m_thread = CreateNativeHttpThread(url, *this, 0, -1, -1, postData); + ASSERT ( m_thread, () ); } virtual ~MemoryHttpRequest() @@ -116,8 +119,11 @@ class FileHttpRequest : public HttpRequest, public IHttpThreadCallback pair range; ChunksDownloadStrategy::ResultT result; 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)); + { + HttpThread * p = CreateNativeHttpThread(url, *this, range.first, range.second, m_progress.second); + ASSERT ( p, () ); + m_threads.push_back(make_pair(p, range.first)); + } return result; } @@ -130,18 +136,27 @@ class FileHttpRequest : public HttpRequest, public IHttpThreadCallback m_threads.erase(it); return; } - ASSERT(false, ("Tried to remove invalid thread?")); + ASSERT ( false, ("Tried to remove invalid thread?") ); } - virtual void OnWrite(int64_t offset, void const * buffer, size_t size) + virtual bool OnWrite(int64_t offset, void const * buffer, size_t size) { #ifdef DEBUG static threads::ThreadID const id = threads::GetCurrentThreadID(); ASSERT_EQUAL(id, threads::GetCurrentThreadID(), ("OnWrite called from different threads")); #endif - m_writer->Seek(offset); - m_writer->Write(buffer, size); + try + { + m_writer->Seek(offset); + m_writer->Write(buffer, size); + return true; + } + catch (Writer::Exception const & ex) + { + LOG(LWARNING, ("Can't write buffer for size = ", size)); + return false; + } } /// Called for each chunk by one main (GUI) thread. @@ -185,7 +200,17 @@ class FileHttpRequest : public HttpRequest, public IHttpThreadCallback if (m_status != EInProgress) { - m_writer.reset(); + try + { + m_writer.reset(); + } + catch (Writer::Exception const & ex) + { + LOG(LWARNING, ("Can't close file correctly. There is not enough space, possibly.")); + + ASSERT_EQUAL ( m_status, EFailed, () ); + m_status = EFailed; + } // clean up resume file with chunks range on success if (m_status == ECompleted) diff --git a/platform/http_thread_callback.hpp b/platform/http_thread_callback.hpp index da3e19810d..3b827bed35 100644 --- a/platform/http_thread_callback.hpp +++ b/platform/http_thread_callback.hpp @@ -8,7 +8,7 @@ namespace downloader class IHttpThreadCallback { public: - virtual void OnWrite(int64_t offset, void const * buffer, size_t size) = 0; + virtual bool OnWrite(int64_t offset, void const * buffer, size_t size) = 0; virtual void OnFinish(long httpCode, int64_t begRange, int64_t endRange) = 0; }; diff --git a/storage/storage.cpp b/storage/storage.cpp index f79a055ff3..eff36a1870 100644 --- a/storage/storage.cpp +++ b/storage/storage.cpp @@ -484,6 +484,7 @@ namespace storage file.m_remoteSize, bind(&Storage::OnMapDownloadFinished, this, _1), bind(&Storage::OnMapDownloadProgress, this, _1))); + ASSERT ( m_request, () ); } string Storage::GetFileDownloadUrl(string const & baseUrl, string const & fName) const