fix crash when redownloading or canceling

qgis · May 7, 2018 · 5610ebe · 5610ebe
1 parent 9d45077
commit 5610ebe
Show file tree

Hide file tree

Showing 3 changed files with 56 additions and 37 deletions.
diff --git a/python/core/qgsnetworkcontentfetcherregistry.sip.in b/python/core/qgsnetworkcontentfetcherregistry.sip.in
@@ -84,6 +84,11 @@ Emitted when the download actually starts
     void cancelTriggered();
 %Docstring
 Emitted when download is canceled.
+%End
+
+    void taskCompleted();
+%Docstring
+Emitted when the download is finished (although file not accessible yet)
 %End
 
 };
@@ -118,7 +123,7 @@ Create the registry for temporary downloaded files
 
     ~QgsNetworkContentFetcherRegistry();
 
-    const QgsFetchedContent *fetch( const QUrl &url, const FetchingMode &fetchingMode = DownloadLater );
+    const QgsFetchedContent *fetch( const QUrl &url, const FetchingMode fetchingMode = DownloadLater );
 %Docstring
  Initialize a download for the given URL
 

diff --git a/src/core/qgsnetworkcontentfetcherregistry.cpp b/src/core/qgsnetworkcontentfetcherregistry.cpp
@@ -19,7 +19,6 @@
 #include "qgsnetworkcontentfetcherregistry.h"
 
 #include "qgsapplication.h"
-#include "qgsnetworkcontentfetchertask.h"
 
 QgsNetworkContentFetcherRegistry::QgsNetworkContentFetcherRegistry()
   : QObject()
@@ -37,7 +36,7 @@ QgsNetworkContentFetcherRegistry::~QgsNetworkContentFetcherRegistry()
   mFileRegistry.clear();
 }
 
-const QgsFetchedContent *QgsNetworkContentFetcherRegistry::fetch( const QUrl &url, const FetchingMode &fetchingMode )
+const QgsFetchedContent *QgsNetworkContentFetcherRegistry::fetch( const QUrl &url, const FetchingMode fetchingMode )
 {
   QMutexLocker locker( &mMutex );
   if ( mFileRegistry.contains( url ) )
@@ -46,72 +45,87 @@ const QgsFetchedContent *QgsNetworkContentFetcherRegistry::fetch( const QUrl &ur
   }
 
   QgsFetchedContent *content = new QgsFetchedContent( nullptr, QgsFetchedContent::NotStarted );
-  content->mFetchingTask = new QgsNetworkContentFetcherTask( url );
 
   // start
   QObject::connect( content, &QgsFetchedContent::downloadStarted, this, [ = ]( const bool redownload )
   {
-    QMutexLocker locker( &mMutex );
-    if ( mFileRegistry.contains( url ) && redownload )
+    if ( redownload && content->status() == QgsFetchedContent::Downloading )
     {
-      QgsFetchedContent *content = mFileRegistry[url];
-      if ( mFileRegistry.value( url )->status() == QgsFetchedContent::Downloading )
       {
-        content->cancel();
+        QMutexLocker locker( &mMutex );
+        if ( content->mFetchingTask )
+          disconnect( content->mFetchingTask, &QgsNetworkContentFetcherTask::fetched, content, &QgsFetchedContent::taskCompleted );
       }
+      // no locker when calling cancel!
+      content->cancel();
     }
-    if ( ( mFileRegistry.contains( url ) && mFileRegistry.value( url )->status() == QgsFetchedContent::NotStarted ) || redownload )
+    QMutexLocker locker( &mMutex );
+    if ( redownload ||
+         content->status() == QgsFetchedContent::NotStarted ||
+         content->status() == QgsFetchedContent::Failed )
     {
+      content->mFetchingTask = new QgsNetworkContentFetcherTask( url );
+      connect( content->mFetchingTask, &QgsNetworkContentFetcherTask::fetched, content, &QgsFetchedContent::taskCompleted );
       QgsApplication::instance()->taskManager()->addTask( content->mFetchingTask );
+      content->mStatus = QgsFetchedContent::Downloading;
     }
   } );
 
   // cancel
   QObject::connect( content, &QgsFetchedContent::cancelTriggered, this, [ = ]()
   {
     QMutexLocker locker( &mMutex );
-    if ( content->mFetchingTask->canCancel() )
+    if ( content->mFetchingTask && content->mFetchingTask->canCancel() )
     {
       content->mFetchingTask->cancel();
     }
     if ( content->mFile )
     {
       content->mFile->deleteLater();
-      mFileRegistry[url]->setFilePath( QStringLiteral() );
+      content->mFilePath = QString();
     }
   } );
 
   // finished
-  QObject::connect( content->mFetchingTask, &QgsNetworkContentFetcherTask::fetched, this, [ = ]()
+  connect( content, &QgsFetchedContent::taskCompleted, this, [ = ]()
   {
     QMutexLocker locker( &mMutex );
-    QNetworkReply *reply = content->mFetchingTask->reply();
-    QgsFetchedContent *content = mFileRegistry.value( url );
-    if ( reply->error() == QNetworkReply::NoError )
+    if ( !content->mFetchingTask || !content->mFetchingTask->reply() )
     {
-      QTemporaryFile *tf = new QTemporaryFile( QStringLiteral( "XXXXXX" ) );
-      content->setFile( tf );
-      tf->open();
-      content->mFile->write( reply->readAll() );
-      // Qt docs notes that on some system if fileName is not called before close, file might get deleted
-      content->setFilePath( tf->fileName() );
-      tf->close();
-      content->setStatus( QgsFetchedContent::Finished );
+      // if no reply, it has been canceled
+      content->mStatus = QgsFetchedContent::Failed;
+      content->mError = QNetworkReply::OperationCanceledError;
+      content->mFilePath = QString();
     }
     else
     {
-      content->setStatus( QgsFetchedContent::Failed );
-      content->setError( reply->error() );
-      content->setFilePath( QStringLiteral() );
+      QNetworkReply *reply = content->mFetchingTask->reply();
+      if ( reply->error() == QNetworkReply::NoError )
+      {
+        QTemporaryFile *tf = new QTemporaryFile( QStringLiteral( "XXXXXX" ) );
+        content->mFile = tf;
+        tf->open();
+        content->mFile->write( reply->readAll() );
+        // Qt docs notes that on some system if fileName is not called before close, file might get deleted
+        content->mFilePath = tf->fileName();
+        tf->close();
+        content->mStatus = QgsFetchedContent::Finished;
+      }
+      else
+      {
+        content->mStatus = QgsFetchedContent::Failed;
+        content->mError = reply->error();
+        content->mFilePath = QString();
+      }
     }
     content->emitFetched();
   } );
 
+  mFileRegistry.insert( url, content );
+
   if ( fetchingMode == DownloadImmediately )
     content->download();
 
-  mFileRegistry.insert( url, content );
-
   return content;
 }
 
@@ -162,7 +176,7 @@ QString QgsNetworkContentFetcherRegistry::localPath( const QString &filePathOrUr
       else
       {
         // if the file is not downloaded yet or has failed, return empty string
-        path = QStringLiteral();
+        path = QString();
       }
     }
     else

diff --git a/src/core/qgsnetworkcontentfetcherregistry.h b/src/core/qgsnetworkcontentfetcherregistry.h
@@ -29,7 +29,8 @@
 
 class QTemporaryFile;
 
-class QgsNetworkContentFetcherTask;
+#include "qgstaskmanager.h"
+#include "qgsnetworkcontentfetchertask.h"
 
 
 /**
@@ -94,15 +95,14 @@ class CORE_EXPORT QgsFetchedContent : public QObject
     //! Emitted when download is canceled.
     void cancelTriggered();
 
+    //! Emitted when the download is finished (although file not accessible yet)
+    void taskCompleted();
+
   private:
-    void setFile( QTemporaryFile *file ) {mFile = file;}
-    void setStatus( ContentStatus status ) {mStatus = status;}
-    void setError( QNetworkReply::NetworkError error ) {mError = error;}
-    void setFilePath( const QString &filePath ) {mFilePath = filePath;}
     void emitFetched() {emit fetched();}
     QTemporaryFile *mFile;
     QString mFilePath = QStringLiteral();
-    QgsNetworkContentFetcherTask *mFetchingTask;
+    QgsNetworkContentFetcherTask *mFetchingTask = nullptr;
     ContentStatus mStatus;
     QNetworkReply::NetworkError mError = QNetworkReply::NoError;
 
@@ -145,7 +145,7 @@ class CORE_EXPORT QgsNetworkContentFetcherRegistry : public QObject
      * \param fetchingMode defines if the download will start immediately or shall be manually triggered
      * \note If the download starts immediately, it will not redownload any already fetched or currently fetching file.
      */
-    const QgsFetchedContent *fetch( const QUrl &url, const FetchingMode &fetchingMode = DownloadLater );
+    const QgsFetchedContent *fetch( const QUrl &url, const FetchingMode fetchingMode = DownloadLater );
 
 #ifndef SIP_RUN