libcalamares: re-do singleton-ness of NetworkAccessManager

The Calamares Manager should not be a singleton; it is needed
in multiple threads (e.g. from QML). It can have a singleton
Private manager that caches and shares things, though.

src/libcalamares/network/Manager.cpp

@@ -22,6 +22,108 @@
 
 #include <algorithm>
 
+/** @brief Does a request asynchronously, returns the (pending) reply
+ *
+ * The extra options for the request are taken from @p options,
+ * including the timeout setting. A timeout will cause the reply
+ * to abort. The reply is **not** scheduled for deletion.
+ *
+ * On failure, returns nullptr (e.g. bad URL, timeout).
+ */
+static QNetworkReply*
+asynchronousRun( QNetworkAccessManager* nam, const QUrl& url, const Calamares::Network::RequestOptions& options )
+{
+    QNetworkRequest request = QNetworkRequest( url );
+    options.applyToRequest( &request );
+
+    QNetworkReply* reply = nam->get( request );
+    QTimer* timer = nullptr;
+
+    // Bail out early if the request is bad
+    if ( reply->error() )
+    {
+        cWarning() << "Early reply error" << reply->error() << reply->errorString();
+        reply->deleteLater();
+        return nullptr;
+    }
+
+    if ( options.hasTimeout() )
+    {
+        timer = new QTimer( reply );
+        timer->setSingleShot( true );
+        QObject::connect( timer, &QTimer::timeout, reply, &QNetworkReply::abort );
+        timer->start( options.timeout() );
+    }
+
+    return reply;
+}
+
+/** @brief Does a request synchronously, returns the request itself
+ *
+ * The extra options for the request are taken from @p options,
+ * including the timeout setting.
+ *
+ * On failure, returns nullptr (e.g. bad URL, timeout). The request
+ * is marked for later automatic deletion, so don't store the pointer.
+ */
+static QPair< Calamares::Network::RequestStatus, QNetworkReply* >
+synchronousRun( QNetworkAccessManager* nam, const QUrl& url, const Calamares::Network::RequestOptions& options )
+{
+    auto* reply = asynchronousRun( nam, url, options );
+    if ( !reply )
+    {
+        cDebug() << "Could not create request for" << url;
+        return qMakePair( Calamares::Network::RequestStatus::Failed, nullptr );
+    }
+
+    QEventLoop loop;
+    QObject::connect( reply, &QNetworkReply::finished, &loop, &QEventLoop::quit );
+    loop.exec();
+    reply->deleteLater();
+    if ( reply->isRunning() )
+    {
+        cDebug() << "Timeout on request for" << url;
+        return qMakePair( Calamares::Network::RequestStatus::Timeout, nullptr );
+    }
+    else if ( reply->error() != QNetworkReply::NoError )
+    {
+        cDebug() << "HTTP error" << reply->error() << "on request for" << url;
+        return qMakePair( Calamares::Network::RequestStatus::HttpError, nullptr );
+    }
+    else
+    {
+        return qMakePair( Calamares::Network::RequestStatus::Ok, reply );
+    }
+}
+
+static Calamares::Network::RequestStatus
+synchronousPing( QNetworkAccessManager* nam, const QUrl& url, const Calamares::Network::RequestOptions& options )
+{
+    if ( !url.isValid() )
+    {
+        return Calamares::Network::RequestStatus::Failed;
+    }
+
+    auto reply = synchronousRun( nam, url, options );
+    if ( reply.first )
+    {
+        return reply.second->bytesAvailable() ? Calamares::Network::RequestStatus::Ok
+                                              : Calamares::Network::RequestStatus::Empty;
+    }
+    else
+    {
+        return reply.first;
+    }
+}
+
+/** @brief Mutex protecting the singleton Private instance */
+static QMutex*
+namMutex()
+{
+    static QMutex namMutex;
+    return &namMutex;
+}
+
 namespace Calamares
 {
 namespace Network
@@ -44,6 +146,14 @@ RequestOptions::applyToRequest( QNetworkRequest* request ) const
     }
 }
 
+/**
+ * @class
+ *
+ * The Private class is intended as a @b singleton and it
+ * holds a QNetworkManager per thread (for network access
+ * from that thread), and also caches have-internet data
+ * to share across Manager instances.
+ */
 class Manager::Private : public QObject
 {
     Q_OBJECT
@@ -53,17 +163,27 @@ private:
     using ThreadNam = QPair< QThread*, QNetworkAccessManager* >;
     QVector< ThreadNam > m_perThreadNams;
 
-public slots:
+    Private();
-    void cleanupNam();
 
-public:
     QVector< QUrl > m_hasInternetUrls;
     bool m_hasInternet = false;
     int m_lastCheckedUrlIndex = -1;
 
-    Private();
+public slots:
+    void cleanupNam();
 
+public:
+    bool hasInternet() const;
+    bool checkHasInternet();
+    void setCheckHasInternetUrl( const QUrl& url );
+    void setCheckHasInternetUrl( const QVector< QUrl >& urls );
+    void addCheckHasInternetUrl( const QUrl& url );
+    QVector< QUrl > getCheckInternetUrls() const;
+
+    /** @brief Returns the NAM for this thread */
     QNetworkAccessManager* nam();
+
+    static Private* instance();
 };
 
 Manager::Private::Private()
@@ -74,11 +194,114 @@ Manager::Private::Private()
     m_perThreadNams.append( qMakePair( QThread::currentThread(), m_nam.get() ) );
 }
 
-static QMutex*
+bool
-namMutex()
+Manager::Private::hasInternet() const
 {
-    static QMutex namMutex;
+    Calamares::MutexLocker lock( namMutex() );
-    return &namMutex;
+    return m_hasInternet;
+}
+
+bool
+Manager::Private::checkHasInternet()
+{
+    // Locks separately
+    auto* threadNAM = nam();
+
+    Calamares::MutexLocker lock( namMutex() );
+
+    if ( m_hasInternetUrls.empty() )
+    {
+        return false;
+    }
+    // It's possible that access was switched off (see below, if the check
+    // fails) so we want to turn it back on first. Otherwise all the
+    // checks will fail **anyway**, defeating the point of the checks.
+#if ( QT_VERSION < QT_VERSION_CHECK( 5, 15, 0 ) )
+    if ( !m_hasInternet )
+    {
+        threadNAM->setNetworkAccessible( QNetworkAccessManager::Accessible );
+    }
+#endif
+    if ( m_lastCheckedUrlIndex < 0 )
+    {
+        m_lastCheckedUrlIndex = 0;
+    }
+    int attempts = 0;
+    do
+    {
+        // Start by pinging the same one as last time
+        m_hasInternet = ::synchronousPing( threadNAM, m_hasInternetUrls.at( m_lastCheckedUrlIndex ), RequestOptions() );
+        // if it's not responding, **then** move on to the next one,
+        // and wrap around if needed
+        if ( !m_hasInternet )
+        {
+            if ( ++( m_lastCheckedUrlIndex ) >= m_hasInternetUrls.size() )
+            {
+                m_lastCheckedUrlIndex = 0;
+            }
+        }
+        // keep track of how often we've tried, because there's no point in
+        // going around more than once.
+        attempts++;
+    } while ( !m_hasInternet && ( attempts < m_hasInternetUrls.size() ) );
+
+// For earlier Qt versions (< 5.15.0), set the accessibility flag to
+// NotAccessible if synchronous ping has failed, so that any module
+// using Qt's networkAccessible method to determine whether or not
+// internet connection is actually available won't get confused.
+#if ( QT_VERSION < QT_VERSION_CHECK( 5, 15, 0 ) )
+    if ( !m_hasInternet )
+    {
+        threadNAM->setNetworkAccessible( QNetworkAccessManager::NotAccessible );
+    }
+#endif
+
+    return m_hasInternet;
+}
+
+void
+Manager::Private::setCheckHasInternetUrl( const QUrl& url )
+{
+    Calamares::MutexLocker lock( namMutex() );
+
+    m_lastCheckedUrlIndex = -1;
+    m_hasInternetUrls.clear();
+    if ( url.isValid() )
+    {
+        m_hasInternetUrls.append( url );
+    }
+}
+
+void
+Manager::Private::setCheckHasInternetUrl( const QVector< QUrl >& urls )
+{
+    Calamares::MutexLocker lock( namMutex() );
+
+    m_lastCheckedUrlIndex = -1;
+    m_hasInternetUrls = urls;
+    auto it = std::remove_if(
+        m_hasInternetUrls.begin(), m_hasInternetUrls.end(), []( const QUrl& u ) { return !u.isValid(); } );
+    if ( it != m_hasInternetUrls.end() )
+    {
+        m_hasInternetUrls.erase( it, m_hasInternetUrls.end() );
+    }
+}
+
+void
+Manager::Private::addCheckHasInternetUrl( const QUrl& url )
+{
+    if ( url.isValid() )
+    {
+        Calamares::MutexLocker lock( namMutex() );
+        m_hasInternetUrls.append( url );
+    }
+}
+
+QVector< QUrl >
+Manager::Private::getCheckInternetUrls() const
+{
+    Calamares::MutexLocker lock( namMutex() );
+    return m_hasInternetUrls;
 }
 
 QNetworkAccessManager*
@@ -127,210 +350,59 @@ Manager::Private::cleanupNam()
     }
 }
 
-Manager::Manager()
+Manager::Private*
-    : d( std::make_unique< Private >() )
+Manager::Private::instance()
 {
+    static auto* p = new Manager::Private;
+    return p;
 }
 
+Manager::Manager() {}
+
 Manager::~Manager() {}
 
-Manager&
-Manager::instance()
-{
-    static auto* s_manager = new Manager();
-    return *s_manager;
-}
-
 bool
 Manager::hasInternet()
 {
-    return d->m_hasInternet;
+    return Private::instance()->hasInternet();
 }
 
 bool
 Manager::checkHasInternet()
 {
-    if ( d->m_hasInternetUrls.empty() )
+    const auto v = Private::instance()->checkHasInternet();
-    {
+    emit hasInternetChanged( v );
-        return false;
+    return v;
-    }
-    // It's possible that access was switched off (see below, if the check
-    // fails) so we want to turn it back on first. Otherwise all the
-    // checks will fail **anyway**, defeating the point of the checks.
-#if ( QT_VERSION < QT_VERSION_CHECK( 5, 15, 0 ) )
-    if ( !d->m_hasInternet )
-    {
-        d->nam()->setNetworkAccessible( QNetworkAccessManager::Accessible );
-    }
-#endif
-    if ( d->m_lastCheckedUrlIndex < 0 )
-    {
-        d->m_lastCheckedUrlIndex = 0;
-    }
-    int attempts = 0;
-    do
-    {
-        // Start by pinging the same one as last time
-        d->m_hasInternet = synchronousPing( d->m_hasInternetUrls.at( d->m_lastCheckedUrlIndex ) );
-        // if it's not responding, **then** move on to the next one,
-        // and wrap around if needed
-        if ( !d->m_hasInternet )
-        {
-            if ( ++( d->m_lastCheckedUrlIndex ) >= d->m_hasInternetUrls.size() )
-            {
-                d->m_lastCheckedUrlIndex = 0;
-            }
-        }
-        // keep track of how often we've tried, because there's no point in
-        // going around more than once.
-        attempts++;
-    } while ( !d->m_hasInternet && ( attempts < d->m_hasInternetUrls.size() ) );
-
-// For earlier Qt versions (< 5.15.0), set the accessibility flag to
-// NotAccessible if synchronous ping has failed, so that any module
-// using Qt's networkAccessible method to determine whether or not
-// internet connection is actually available won't get confused.
-#if ( QT_VERSION < QT_VERSION_CHECK( 5, 15, 0 ) )
-    if ( !d->m_hasInternet )
-    {
-        d->nam()->setNetworkAccessible( QNetworkAccessManager::NotAccessible );
-    }
-#endif
-
-    emit hasInternetChanged( d->m_hasInternet );
-    return d->m_hasInternet;
 }
 
 void
 Manager::setCheckHasInternetUrl( const QUrl& url )
 {
-    d->m_lastCheckedUrlIndex = -1;
+    Private::instance()->setCheckHasInternetUrl( url );
-    d->m_hasInternetUrls.clear();
-    if ( url.isValid() )
-    {
-        d->m_hasInternetUrls.append( url );
-    }
 }
 
 void
 Manager::setCheckHasInternetUrl( const QVector< QUrl >& urls )
 {
-    d->m_lastCheckedUrlIndex = -1;
+    Private::instance()->setCheckHasInternetUrl( urls );
-    d->m_hasInternetUrls = urls;
-    auto it = std::remove_if(
-        d->m_hasInternetUrls.begin(), d->m_hasInternetUrls.end(), []( const QUrl& u ) { return !u.isValid(); } );
-    if ( it != d->m_hasInternetUrls.end() )
-    {
-        d->m_hasInternetUrls.erase( it, d->m_hasInternetUrls.end() );
-    }
 }
 
 void
 Manager::addCheckHasInternetUrl( const QUrl& url )
 {
-    if ( url.isValid() )
+    Private::instance()->addCheckHasInternetUrl( url );
-    {
-        d->m_hasInternetUrls.append( url );
-    }
 }
 
 QVector< QUrl >
-Manager::getCheckInternetUrls() const
+Manager::getCheckInternetUrls()
 {
-    return d->m_hasInternetUrls;
+    return Private::instance()->getCheckInternetUrls();
-}
-
-/** @brief Does a request asynchronously, returns the (pending) reply
- *
- * The extra options for the request are taken from @p options,
- * including the timeout setting. A timeout will cause the reply
- * to abort. The reply is **not** scheduled for deletion.
- *
- * On failure, returns nullptr (e.g. bad URL, timeout).
- */
-static QNetworkReply*
-asynchronousRun( QNetworkAccessManager* nam, const QUrl& url, const RequestOptions& options )
-{
-    QNetworkRequest request = QNetworkRequest( url );
-    options.applyToRequest( &request );
-
-    QNetworkReply* reply = nam->get( request );
-    QTimer* timer = nullptr;
-
-    // Bail out early if the request is bad
-    if ( reply->error() )
-    {
-        cWarning() << "Early reply error" << reply->error() << reply->errorString();
-        reply->deleteLater();
-        return nullptr;
-    }
-
-    if ( options.hasTimeout() )
-    {
-        timer = new QTimer( reply );
-        timer->setSingleShot( true );
-        QObject::connect( timer, &QTimer::timeout, reply, &QNetworkReply::abort );
-        timer->start( options.timeout() );
-    }
-
-    return reply;
-}
-
-/** @brief Does a request synchronously, returns the request itself
- *
- * The extra options for the request are taken from @p options,
- * including the timeout setting.
- *
- * On failure, returns nullptr (e.g. bad URL, timeout). The request
- * is marked for later automatic deletion, so don't store the pointer.
- */
-static QPair< RequestStatus, QNetworkReply* >
-synchronousRun( QNetworkAccessManager* nam, const QUrl& url, const RequestOptions& options )
-{
-    auto* reply = asynchronousRun( nam, url, options );
-    if ( !reply )
-    {
-        cDebug() << "Could not create request for" << url;
-        return qMakePair( RequestStatus( RequestStatus::Failed ), nullptr );
-    }
-
-    QEventLoop loop;
-    QObject::connect( reply, &QNetworkReply::finished, &loop, &QEventLoop::quit );
-    loop.exec();
-    reply->deleteLater();
-    if ( reply->isRunning() )
-    {
-        cDebug() << "Timeout on request for" << url;
-        return qMakePair( RequestStatus( RequestStatus::Timeout ), nullptr );
-    }
-    else if ( reply->error() != QNetworkReply::NoError )
-    {
-        cDebug() << "HTTP error" << reply->error() << "on request for" << url;
-        return qMakePair( RequestStatus( RequestStatus::HttpError ), nullptr );
-    }
-    else
-    {
-        return qMakePair( RequestStatus( RequestStatus::Ok ), reply );
-    }
 }
 
 RequestStatus
 Manager::synchronousPing( const QUrl& url, const RequestOptions& options )
 {
-    if ( !url.isValid() )
+    return ::synchronousPing( Private::instance()->nam(), url, options );
-    {
-        return RequestStatus::Failed;
-    }
-
-    auto reply = synchronousRun( d->nam(), url, options );
-    if ( reply.first )
-    {
-        return reply.second->bytesAvailable() ? RequestStatus::Ok : RequestStatus::Empty;
-    }
-    else
-    {
-        return reply.first;
-    }
 }
 
 QByteArray
@@ -341,14 +413,14 @@ Manager::synchronousGet( const QUrl& url, const RequestOptions& options )
         return QByteArray();
     }
 
-    auto reply = synchronousRun( d->nam(), url, options );
+    auto reply = synchronousRun( Private::instance()->nam(), url, options );
     return reply.first ? reply.second->readAll() : QByteArray();
 }
 
 QNetworkReply*
 Manager::asynchronousGet( const QUrl& url, const Calamares::Network::RequestOptions& options )
 {
-    return asynchronousRun( d->nam(), url, options );
+    return asynchronousRun( Private::instance()->nam(), url, options );
 }
 
 QDebug&

src/libcalamares/network/Manager.h

@@ -92,15 +92,8 @@ class DLLEXPORT Manager : public QObject
     Q_PROPERTY( bool hasInternet READ hasInternet NOTIFY hasInternetChanged FINAL )
     Q_PROPERTY( QVector< QUrl > checkInternetUrls READ getCheckInternetUrls WRITE setCheckHasInternetUrl )
 
-    Manager();
-
 public:
-    /** @brief Gets the single Manager instance.
+    Manager();
-     *
-     * Typical code will use `auto& nam = Manager::instance();`
-     * to keep the reference.
-     */
-    static Manager& instance();
     ~Manager() override;
 
     /** @brief Checks if the given @p url returns data.
@@ -121,18 +114,6 @@ public:
      */
     QByteArray synchronousGet( const QUrl& url, const RequestOptions& options = RequestOptions() );
 
-    /// @brief Set the URL which is used for the general "is there internet" check.
-    void setCheckHasInternetUrl( const QUrl& url );
-
-    /// @brief Adds an (extra) URL to check
-    void addCheckHasInternetUrl( const QUrl& url );
-
-    /// @brief Set a collection of URLs used for the general "is there internet" check.
-    void setCheckHasInternetUrl( const QVector< QUrl >& urls );
-
-    /// @brief What URLs are used to check for internet connectivity?
-    QVector< QUrl > getCheckInternetUrls() const;
-
     /** @brief Do a network request asynchronously.
      *
      * Returns a pointer to the reply-from-the-request.
@@ -141,6 +122,18 @@ public:
      */
     QNetworkReply* asynchronousGet( const QUrl& url, const RequestOptions& options = RequestOptions() );
 
+    /// @brief Set the URL which is used for the general "is there internet" check.
+    static void setCheckHasInternetUrl( const QUrl& url );
+
+    /// @brief Adds an (extra) URL to check
+    static void addCheckHasInternetUrl( const QUrl& url );
+
+    /// @brief Set a collection of URLs used for the general "is there internet" check.
+    static void setCheckHasInternetUrl( const QVector< QUrl >& urls );
+
+    /// @brief What URLs are used to check for internet connectivity?
+    static QVector< QUrl > getCheckInternetUrls();
+
 public Q_SLOTS:
     /** @brief Do an explicit check for internet connectivity.
      *
@@ -167,7 +160,6 @@ signals:
 
 private:
     class Private;
-    std::unique_ptr< Private > d;
 };
 }  // namespace Network
 }  // namespace Calamares

src/libcalamares/network/Tests.cpp

@@ -28,7 +28,7 @@ NetworkTests::initTestCase()
 void
 NetworkTests::testInstance()
 {
-    auto& nam = Calamares::Network::Manager::instance();
+    Calamares::Network::Manager nam;
     QVERIFY( !nam.hasInternet() );
     QCOMPARE( nam.getCheckInternetUrls().count(), 0 );
 }
@@ -38,7 +38,7 @@ NetworkTests::testPing()
 {
     using namespace Calamares::Network;
     Logger::setupLogLevel( Logger::LOGVERBOSE );
-    auto& nam = Manager::instance();
+    Manager nam;
 
     // On FreeBSD, the SSL handling depends on the presence of root keys
     // (from the ca_nss port) which may not be available. So HTTPS requests
@@ -67,7 +67,7 @@ NetworkTests::testCheckUrl()
 {
     using namespace Calamares::Network;
     Logger::setupLogLevel( Logger::LOGVERBOSE );
-    auto& nam = Manager::instance();
+    Manager nam;
 
     {
         QUrl u( "http://example.com" );
@@ -97,7 +97,7 @@ NetworkTests::testCheckMultiUrl()
 {
     using namespace Calamares::Network;
     Logger::setupLogLevel( Logger::LOGVERBOSE );
-    auto& nam = Manager::instance();
+    Manager nam;
 
     {
         QUrl u0( "http://example.com" );