diff --git a/src/libcalamares/network/Manager.cpp b/src/libcalamares/network/Manager.cpp index 275a7f523..b319b58c8 100644 --- a/src/libcalamares/network/Manager.cpp +++ b/src/libcalamares/network/Manager.cpp @@ -22,6 +22,108 @@ #include +/** @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: - void cleanupNam(); + Private(); -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* -namMutex() +bool +Manager::Private::hasInternet() const { - static QMutex namMutex; - return &namMutex; + Calamares::MutexLocker lock( 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() - : d( std::make_unique< Private >() ) +Manager::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() ) - { - 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 ( !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; + const auto v = Private::instance()->checkHasInternet(); + emit hasInternetChanged( v ); + return v; } void Manager::setCheckHasInternetUrl( const QUrl& url ) { - d->m_lastCheckedUrlIndex = -1; - d->m_hasInternetUrls.clear(); - if ( url.isValid() ) - { - d->m_hasInternetUrls.append( url ); - } + Private::instance()->setCheckHasInternetUrl( url ); } void Manager::setCheckHasInternetUrl( const QVector< QUrl >& urls ) { - d->m_lastCheckedUrlIndex = -1; - 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() ); - } + Private::instance()->setCheckHasInternetUrl( urls ); } void Manager::addCheckHasInternetUrl( const QUrl& url ) { - if ( url.isValid() ) - { - d->m_hasInternetUrls.append( url ); - } + Private::instance()->addCheckHasInternetUrl( url ); } QVector< QUrl > -Manager::getCheckInternetUrls() const +Manager::getCheckInternetUrls() { - return d->m_hasInternetUrls; -} - -/** @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 ); - } + return Private::instance()->getCheckInternetUrls(); } RequestStatus Manager::synchronousPing( const QUrl& url, const RequestOptions& options ) { - if ( !url.isValid() ) - { - 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; - } + return ::synchronousPing( Private::instance()->nam(), url, options ); } 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& diff --git a/src/libcalamares/network/Manager.h b/src/libcalamares/network/Manager.h index 2538b35c5..9afb706e4 100644 --- a/src/libcalamares/network/Manager.h +++ b/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. - * - * Typical code will use `auto& nam = Manager::instance();` - * to keep the reference. - */ - static Manager& instance(); + Manager(); ~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 diff --git a/src/libcalamares/network/Tests.cpp b/src/libcalamares/network/Tests.cpp index 284fcba75..cba49ed23 100644 --- a/src/libcalamares/network/Tests.cpp +++ b/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" );