QgsSvgCache fetches remote SVG files in a background task

Previously QgsSvgCache would often try to fetch remote images using a network request on the main thread, by calling processEvents repeatedly until the request was complete. This caused lots of bugs, since the main thread processEvents would proceed with all kinds of stuff assuming that the svg fetch operation was complete, leading to frequent crashes and deadlocks and making remote svg use impossible (it's likely that the SVG cache remote fetching code was written in the pre-multi-threaded rendering era). There's no way to fix this with async svg fetching - we HAVE to remove the processEvents call, and a QEventLoop won't help either (since the method may be called on the main thread). Accordingly the only solution is to fetch the requested svg in the background, and return a temporary "downloading" svg for use in the meantime. We use a QgsNetworkContentFetcherTask to do this, so it's nicely integrated with task manager. A request task is fired up when a remote svg is requested for the first time, with the temporary downloading svg returned for use by the caller asynchronously. QgsSvgCache then emits the remoteSvgFetched signal when a previously requested remote SVG has been successfully fetched, triggering a map canvas redraw with the correct SVG graphic. Fixes #18504
qgis · Apr 9, 2018 · 45c400c · 45c400c
1 parent 3dec175
commit 45c400c
Show file tree

Hide file tree

Showing 6 changed files with 113 additions and 67 deletions.
diff --git a/python/core/symbology/qgssvgcache.sip.in b/python/core/symbology/qgssvgcache.sip.in
@@ -138,6 +138,13 @@ Get SVG content
     void statusChanged( const QString  &statusQString );
 %Docstring
 Emit a signal to be caught by qgisapp and display a msg on status bar
+%End
+
+    void remoteSvgFetched( const QString &url );
+%Docstring
+Emitted when the cache has finished retrieving an SVG file from a remote ``url``.
+
+.. versionadded:: 3.2
 %End
 
 };

diff --git a/python/gui/qgsmapcanvas.sip.in b/python/gui/qgsmapcanvas.sip.in
@@ -112,13 +112,6 @@ Check whether images of rendered layers are curerently being cached
 Make sure to remove any rendered images from cache (does nothing if cache is not enabled)
 
 .. versionadded:: 2.4
-%End
-
-    void refreshAllLayers();
-%Docstring
-Reload all layers, clear the cache and refresh the canvas
-
-.. versionadded:: 2.9
 %End
 
     void waitWhileRendering();
@@ -715,6 +708,13 @@ for the canvas.
     void refresh();
 %Docstring
 Repaints the canvas map
+%End
+
+    void refreshAllLayers();
+%Docstring
+Reload all layers, clear the cache and refresh the canvas
+
+.. versionadded:: 2.9
 %End
 
     void selectionChangedSlot();

diff --git a/src/core/symbology/qgssvgcache.cpp b/src/core/symbology/qgssvgcache.cpp
@@ -21,6 +21,7 @@
 #include "qgsnetworkaccessmanager.h"
 #include "qgsmessagelog.h"
 #include "qgssymbollayerutils.h"
+#include "qgsnetworkcontentfetchertask.h"
 
 #include <QApplication>
 #include <QCoreApplication>
@@ -80,8 +81,10 @@ int QgsSvgCacheEntry::dataSize() const
 
 QgsSvgCache::QgsSvgCache( QObject *parent )
   : QObject( parent )
+  , mMutex( QMutex::Recursive )
 {
   mMissingSvg = QStringLiteral( "<svg width='10' height='10'><text x='5' y='10' font-size='10' text-anchor='middle'>?</text></svg>" ).toLatin1();
+  mFetchingSvg = QStringLiteral( "<svg width='10' height='10'><text x='5' y='10' font-size='10' text-anchor='middle'>x</text></svg>" ).toLatin1();
 }
 
 QgsSvgCache::~QgsSvgCache()
@@ -406,77 +409,70 @@ QByteArray QgsSvgCache::getImageData( const QString &path ) const
     return mMissingSvg;
   }
 
-  // the url points to a remote resource, download it!
-  QNetworkReply *reply = nullptr;
+  QMutexLocker locker( &mMutex );
+
+  // already a request in progress for this url
+  if ( mPendingRemoteUrls.contains( path ) )
+    return mFetchingSvg;
 
-  // The following code blocks until the file is downloaded...
-  // TODO: use signals to get reply finished notification, in this moment
-  // it's executed while rendering.
-  while ( true )
+  if ( mRemoteContentCache.contains( path ) )
   {
-    QgsDebugMsg( QString( "get svg: %1" ).arg( svgUrl.toString() ) );
-    QNetworkRequest request( svgUrl );
-    request.setAttribute( QNetworkRequest::CacheLoadControlAttribute, QNetworkRequest::PreferCache );
-    request.setAttribute( QNetworkRequest::CacheSaveControlAttribute, true );
+    // already fetched this content - phew. Just return what we already got.
+    return *mRemoteContentCache[ path ];
+  }
 
-    reply = QgsNetworkAccessManager::instance()->get( request );
-    connect( reply, &QNetworkReply::downloadProgress, this, &QgsSvgCache::downloadProgress );
+  mPendingRemoteUrls.insert( path );
+  //fire up task to fetch image in background
+  QNetworkRequest request( svgUrl );
+  request.setAttribute( QNetworkRequest::CacheLoadControlAttribute, QNetworkRequest::PreferCache );
+  request.setAttribute( QNetworkRequest::CacheSaveControlAttribute, true );
 
-    //emit statusChanged( tr( "Downloading svg." ) );
+  QgsNetworkContentFetcherTask *task = new QgsNetworkContentFetcherTask( request );
+  connect( task, &QgsNetworkContentFetcherTask::fetched, this, [this, task, path]
+  {
+    QMutexLocker locker( &mMutex );
 
-    // wait until the image download finished
-    // TODO: connect to the reply->finished() signal
-    while ( !reply->isFinished() )
+    QNetworkReply *reply = task->reply();
+    if ( !reply )
     {
-      QCoreApplication::processEvents( QEventLoop::ExcludeUserInputEvents, 500 );
+      // cancelled
+      QMetaObject::invokeMethod( const_cast< QgsSvgCache * >( this ), "onRemoteSvgFetched", Qt::QueuedConnection, Q_ARG( QString, path ), Q_ARG( bool, false ) );
+      return;
     }
 
     if ( reply->error() != QNetworkReply::NoError )
     {
       QgsMessageLog::logMessage( tr( "SVG request failed [error: %1 - url: %2]" ).arg( reply->errorString(), path ), tr( "SVG" ) );
-      reply->deleteLater();
-      return mMissingSvg;
+      return;
     }
 
-    QVariant redirect = reply->attribute( QNetworkRequest::RedirectionTargetAttribute );
-    if ( redirect.isNull() )
+    QVariant status = reply->attribute( QNetworkRequest::HttpStatusCodeAttribute );
+    if ( !status.isNull() && status.toInt() >= 400 )
     {
-      // neither network error nor redirection
-      // TODO: cache the image
-      break;
+      QVariant phrase = reply->attribute( QNetworkRequest::HttpReasonPhraseAttribute );
+      QgsMessageLog::logMessage( tr( "SVG request error [status: %1 - reason phrase: %2] for %3" ).arg( status.toInt() ).arg( phrase.toString(), path ), tr( "SVG" ) );
+      mRemoteContentCache.insert( path, new QByteArray( mMissingSvg ) );
+      return;
     }
 
-    // do a new request to the redirect url
-    svgUrl = redirect.toUrl();
-    reply->deleteLater();
-  }
-
-  QVariant status = reply->attribute( QNetworkRequest::HttpStatusCodeAttribute );
-  if ( !status.isNull() && status.toInt() >= 400 )
-  {
-    QVariant phrase = reply->attribute( QNetworkRequest::HttpReasonPhraseAttribute );
-    QgsMessageLog::logMessage( tr( "SVG request error [status: %1 - reason phrase: %2] for %3" ).arg( status.toInt() ).arg( phrase.toString(), path ), tr( "SVG" ) );
-
-    reply->deleteLater();
-    return mMissingSvg;
-  }
-
-  // we accept both real SVG mime types AND plain text types - because some sites
-  // (notably github) serve up svgs as raw text
-  QString contentType = reply->header( QNetworkRequest::ContentTypeHeader ).toString();
-  if ( !contentType.startsWith( QLatin1String( "image/svg+xml" ), Qt::CaseInsensitive )
-       && !contentType.startsWith( QLatin1String( "text/plain" ), Qt::CaseInsensitive ) )
-  {
-    QgsMessageLog::logMessage( tr( "Unexpected MIME type %1 received for %2" ).arg( contentType, path ), tr( "SVG" ) );
-    reply->deleteLater();
-    return mMissingSvg;
-  }
+    // we accept both real SVG mime types AND plain text types - because some sites
+    // (notably github) serve up svgs as raw text
+    QString contentType = reply->header( QNetworkRequest::ContentTypeHeader ).toString();
+    if ( !contentType.startsWith( QLatin1String( "image/svg+xml" ), Qt::CaseInsensitive )
+         && !contentType.startsWith( QLatin1String( "text/plain" ), Qt::CaseInsensitive ) )
+    {
+      QgsMessageLog::logMessage( tr( "Unexpected MIME type %1 received for %2" ).arg( contentType, path ), tr( "SVG" ) );
+      mRemoteContentCache.insert( path, new QByteArray( mMissingSvg ) );
+      return;
+    }
 
-  // read the image data
-  QByteArray ba = reply->readAll();
-  reply->deleteLater();
+    // read the image data
+    mRemoteContentCache.insert( path, new QByteArray( reply->readAll() ) );
+    QMetaObject::invokeMethod( const_cast< QgsSvgCache * >( this ), "onRemoteSvgFetched", Qt::QueuedConnection, Q_ARG( QString, path ), Q_ARG( bool, true ) );
+  } );
 
-  return ba;
+  QgsApplication::taskManager()->addTask( task );
+  return mFetchingSvg;
 }
 
 void QgsSvgCache::cacheImage( QgsSvgCacheEntry *entry )
@@ -998,3 +994,25 @@ void QgsSvgCache::downloadProgress( qint64 bytesReceived, qint64 bytesTotal )
   QgsDebugMsg( msg );
   emit statusChanged( msg );
 }
+
+void QgsSvgCache::onRemoteSvgFetched( const QString &url, bool success )
+{
+  QMutexLocker locker( &mMutex );
+  mPendingRemoteUrls.remove( url );
+
+  QgsSvgCacheEntry *nextEntry = mLeastRecentEntry;
+  while ( QgsSvgCacheEntry *entry = nextEntry )
+  {
+    nextEntry = entry->nextEntry;
+    if ( entry->path == url )
+    {
+      takeEntryFromList( entry );
+      mEntryLookup.remove( entry->path, entry );
+      mTotalSize -= entry->dataSize();
+      delete entry;
+    }
+  }
+
+  if ( success )
+    emit remoteSvgFetched( url );
+}
diff --git a/src/core/symbology/qgssvgcache.h b/src/core/symbology/qgssvgcache.h
@@ -31,6 +31,8 @@
 #include <QElapsedTimer>
 #include <QPicture>
 #include <QImage>
+#include <QCache>
+#include <QSet>
 
 #include "qgis_core.h"
 
@@ -226,9 +228,17 @@ class CORE_EXPORT QgsSvgCache : public QObject
     //! Emit a signal to be caught by qgisapp and display a msg on status bar
     void statusChanged( const QString  &statusQString );
 
+    /**
+     * Emitted when the cache has finished retrieving an SVG file from a remote \a url.
+     * \since QGIS 3.2
+     */
+    void remoteSvgFetched( const QString &url );
+
   private slots:
     void downloadProgress( qint64, qint64 );
 
+    void onRemoteSvgFetched( const QString &url, bool success );
+
   private:
 
     /**
@@ -303,11 +313,18 @@ class CORE_EXPORT QgsSvgCache : public QObject
      */
     QImage imageFromCachedPicture( const QgsSvgCacheEntry &entry ) const;
 
+    QByteArray fetchImageData( const QString &path, bool &ok ) const;
+
     //! SVG content to be rendered if SVG file was not found.
     QByteArray mMissingSvg;
 
+    QByteArray mFetchingSvg;
+
     //! Mutex to prevent concurrent access to the class from multiple threads at once (may corrupt the entries otherwise).
-    QMutex mMutex;
+    mutable QMutex mMutex;
+
+    mutable QCache< QString, QByteArray > mRemoteContentCache;
+    mutable QSet< QString > mPendingRemoteUrls;
 
     friend class TestQgsSvgCache;
 };

diff --git a/src/gui/qgsmapcanvas.cpp b/src/gui/qgsmapcanvas.cpp
@@ -65,6 +65,7 @@ email                : sherman at mrcc.com
 #include "qgsvectorlayer.h"
 #include "qgsmapthemecollection.h"
 #include "qgscoordinatetransformcontext.h"
+#include "qgssvgcache.h"
 #include <cmath>
 
 /**
@@ -148,6 +149,9 @@ QgsMapCanvas::QgsMapCanvas( QWidget *parent )
     refresh();
   } );
 
+  // refresh canvas when a remote svg has finished downloading
+  connect( QgsApplication::svgCache(), &QgsSvgCache::remoteSvgFetched, this, &QgsMapCanvas::refreshAllLayers );
+
   //segmentation parameters
   QgsSettings settings;
   double segmentationTolerance = settings.value( QStringLiteral( "qgis/segmentationTolerance" ), "0.01745" ).toDouble();

diff --git a/src/gui/qgsmapcanvas.h b/src/gui/qgsmapcanvas.h
@@ -157,12 +157,6 @@ class GUI_EXPORT QgsMapCanvas : public QGraphicsView
      */
     void clearCache();
 
-    /**
-     * Reload all layers, clear the cache and refresh the canvas
-     * \since QGIS 2.9
-     */
-    void refreshAllLayers();
-
     /**
      * Blocks until the rendering job has finished.
      *
@@ -638,6 +632,12 @@ class GUI_EXPORT QgsMapCanvas : public QGraphicsView
     //! Repaints the canvas map
     void refresh();
 
+    /**
+     * Reload all layers, clear the cache and refresh the canvas
+     * \since QGIS 2.9
+     */
+    void refreshAllLayers();
+
     //! Receives signal about selection change, and pass it on with layer info
     void selectionChangedSlot();