From 6250a19a1f459f764af29a5998bf8341704b05ed Mon Sep 17 00:00:00 2001 From: Anton Romanov Date: Thu, 14 May 2015 17:29:03 -0700 Subject: [PATCH 1/3] Tie timeouts to resolvers, makes pipeline more predictable, stable and deterministic --- src/libtomahawk/Pipeline.cpp | 86 ++++++++----------- src/libtomahawk/Pipeline.h | 12 +-- src/libtomahawk/Pipeline_p.h | 3 +- src/libtomahawk/database/DatabaseImpl.cpp | 2 +- src/libtomahawk/database/DatabaseResolver.cpp | 2 +- .../resolvers/JSResolverHelper.cpp | 2 +- src/libtomahawk/resolvers/ScriptResolver.cpp | 2 +- src/libtomahawk/utils/ResultUrlChecker.cpp | 4 +- src/libtomahawk/utils/ResultUrlChecker.h | 4 +- 9 files changed, 52 insertions(+), 65 deletions(-) diff --git a/src/libtomahawk/Pipeline.cpp b/src/libtomahawk/Pipeline.cpp index 5d38b38c5..abfd179c7 100644 --- a/src/libtomahawk/Pipeline.cpp +++ b/src/libtomahawk/Pipeline.cpp @@ -36,6 +36,7 @@ #define MAX_CONCURRENT_QUERIES 16 #define CLEANUP_TIMEOUT 5 * 60 * 1000 #define MINSCORE 0.5 +#define DEFAULT_RESOLVER_TIMEOUT 5000 //5 seconds using namespace Tomahawk; @@ -99,7 +100,7 @@ unsigned int Pipeline::activeQueryCount() const { Q_D( const Pipeline ); - return d->qidsState.count(); + return d->qidsState.uniqueKeys().count(); } @@ -323,7 +324,7 @@ Pipeline::resolve( QID qid, bool prioritized, bool temporaryQuery ) void -Pipeline::reportResults( QID qid, const QList< result_ptr >& results ) +Pipeline::reportResults( QID qid, Tomahawk::Resolver* r, const QList< result_ptr >& results ) { Q_D( Pipeline ); if ( !d->running ) @@ -366,17 +367,17 @@ Pipeline::reportResults( QID qid, const QList< result_ptr >& results ) addResultsToQuery( q, cleanResults ); if ( !httpResults.isEmpty() ) { - const ResultUrlChecker* checker = new ResultUrlChecker( q, httpResults ); + const ResultUrlChecker* checker = new ResultUrlChecker( q, r, httpResults ); connect( checker, SIGNAL( done() ), SLOT( onResultUrlCheckerDone() ) ); } else { - decQIDState( q ); + decQIDState( q, r ); } /* if ( q->solved() && !q->isFullTextQuery() ) { - setQIDState( q, 0 ); + checkQIDState( q, 0 ); return; }*/ } @@ -413,7 +414,7 @@ Pipeline::addResultsToQuery( const query_ptr& query, const QList< result_ptr >& void -Pipeline::onResultUrlCheckerDone() +Pipeline::onResultUrlCheckerDone( ) { ResultUrlChecker* checker = qobject_cast< ResultUrlChecker* >( sender() ); if ( !checker ) @@ -425,11 +426,11 @@ Pipeline::onResultUrlCheckerDone() addResultsToQuery( q, checker->validResults() ); /* if ( q && !q->isFullTextQuery() ) { - setQIDState( q, 0 ); + checkQIDState( q, 0 ); return; }*/ - decQIDState( q ); + decQIDState( q, reinterpret_cast( checker->resolver() ) ); } @@ -525,26 +526,22 @@ Pipeline::shuntNext() q->setCurrentResolver( 0 ); } - setQIDState( q, rc ); + foreach ( Resolver* r, d->resolvers ) + { + incQIDState( q, r ); + } + checkQIDState( q ); } void -Pipeline::timeoutShunt( const query_ptr& q ) +Pipeline::timeoutShunt( const query_ptr& q, Tomahawk::Resolver* r ) { Q_D( Pipeline ); if ( !d->running ) return; - // are we still waiting for a timeout? - if ( d->qidsTimeout.contains( q->id() ) ) - { - if ( --d->qidsTimeout[q->id()] == 0 ) - { - d->qidsTimeout.remove( q->id() ); - setQIDState( q, 0 ); - } - } + decQIDState( q, r ); } @@ -567,16 +564,15 @@ Pipeline::shunt( const query_ptr& q ) r->resolve( q ); emit resolving( q ); - if ( r->timeout() > 0 ) - { - d->qidsTimeout[q->id()]++; - new FuncTimeout( r->timeout(), std::bind( &Pipeline::timeoutShunt, this, q ), this ); - } + auto timeout = r->timeout(); + if ( timeout == 0 ) + timeout = DEFAULT_RESOLVER_TIMEOUT; + new FuncTimeout( r->timeout(), std::bind( &Pipeline::timeoutShunt, this, q, r ), this ); } else { // we get here if we disable a resolver while a query is resolving - setQIDState( q, 0 ); + decQIDState(q, r); return; } @@ -610,15 +606,15 @@ Pipeline::nextResolver( const Tomahawk::query_ptr& query ) const void -Pipeline::setQIDState( const Tomahawk::query_ptr& query, int state ) +Pipeline::checkQIDState( const Tomahawk::query_ptr& query ) { Q_D( Pipeline ); QMutexLocker lock( &d->mut ); - if ( state > 0 ) - { - d->qidsState.insert( query->id(), state ); + tDebug() << Q_FUNC_INFO << " " << query->id() << " " << d->qidsState.count( query->id() ); + if ( d->qidsState.contains( query->id() ) ) + { new FuncTimeout( 0, std::bind( &Pipeline::shunt, this, query ), this ); } else @@ -634,39 +630,27 @@ Pipeline::setQIDState( const Tomahawk::query_ptr& query, int state ) } -int -Pipeline::incQIDState( const Tomahawk::query_ptr& query ) +void +Pipeline::incQIDState( const Tomahawk::query_ptr& query, Tomahawk::Resolver* r ) { Q_D( Pipeline ); QMutexLocker lock( &d->mut ); - int state = 1; - if ( d->qidsState.contains( query->id() ) ) - { - state = d->qidsState.value( query->id() ) + 1; - } - d->qidsState.insert( query->id(), state ); - - return state; + d->qidsState.insert( query->id(), r ); } -int -Pipeline::decQIDState( const Tomahawk::query_ptr& query ) +void +Pipeline::decQIDState( const Tomahawk::query_ptr& query, Tomahawk::Resolver* r ) { Q_D( Pipeline ); - int state = 0; - { - QMutexLocker lock( &d->mut ); - if ( !d->qidsState.contains( query->id() ) ) - return 0; + if ( r ) + d->qidsState.remove( query->id(), r );//Removes all matching pairs + else + d->qidsState.remove( query->id() );//Will clear - state = d->qidsState.value( query->id() ) - 1; - } - - setQIDState( query, state ); - return state; + checkQIDState( query ); } diff --git a/src/libtomahawk/Pipeline.h b/src/libtomahawk/Pipeline.h index cfab80796..83ec38756 100644 --- a/src/libtomahawk/Pipeline.h +++ b/src/libtomahawk/Pipeline.h @@ -54,7 +54,7 @@ public: unsigned int pendingQueryCount() const; unsigned int activeQueryCount() const; - void reportResults( QID qid, const QList< result_ptr >& results ); + void reportResults( QID qid, Tomahawk::Resolver* r, const QList< result_ptr >& results ); void reportAlbums( QID qid, const QList< album_ptr >& albums ); void reportArtists( QID qid, const QList< artist_ptr >& artists ); @@ -94,12 +94,12 @@ protected: QScopedPointer d_ptr; private slots: - void timeoutShunt( const query_ptr& q ); + void timeoutShunt( const query_ptr& q, Tomahawk::Resolver* r ); void shunt( const query_ptr& q ); void shuntNext(); void onTemporaryQueryTimer(); - void onResultUrlCheckerDone(); + void onResultUrlCheckerDone( ); private: Q_DECLARE_PRIVATE( Pipeline ) @@ -107,9 +107,9 @@ private: void addResultsToQuery( const query_ptr& query, const QList< result_ptr >& results ); Tomahawk::Resolver* nextResolver( const Tomahawk::query_ptr& query ) const; - void setQIDState( const Tomahawk::query_ptr& query, int state ); - int incQIDState( const Tomahawk::query_ptr& query ); - int decQIDState( const Tomahawk::query_ptr& query ); + void checkQIDState( const Tomahawk::query_ptr& query ); + void incQIDState( const Tomahawk::query_ptr& query, Tomahawk::Resolver* ); + void decQIDState( const Tomahawk::query_ptr& query, Tomahawk::Resolver* ); }; } // Tomahawk diff --git a/src/libtomahawk/Pipeline_p.h b/src/libtomahawk/Pipeline_p.h index bbcc9f7f5..571fe357c 100644 --- a/src/libtomahawk/Pipeline_p.h +++ b/src/libtomahawk/Pipeline_p.h @@ -45,8 +45,7 @@ private: QList< Resolver* > resolvers; QList< QPointer > scriptResolvers; QList< ResolverFactoryFunc > resolverFactories; - QMap< QID, unsigned int > qidsTimeout; - QMap< QID, unsigned int > qidsState; + QMultiMap< QID, Tomahawk::Resolver* > qidsState; QMap< QID, query_ptr > qids; QMap< RID, result_ptr > rids; diff --git a/src/libtomahawk/database/DatabaseImpl.cpp b/src/libtomahawk/database/DatabaseImpl.cpp index c7ed9af97..9626e8900 100644 --- a/src/libtomahawk/database/DatabaseImpl.cpp +++ b/src/libtomahawk/database/DatabaseImpl.cpp @@ -644,7 +644,7 @@ Tomahawk::DatabaseImpl::resultFromHint( const Tomahawk::query_ptr& origquery ) const QUrl u = QUrl::fromUserInput( url ); res->setFriendlySource( u.host() ); - ResultUrlChecker* checker = new ResultUrlChecker( origquery, QList< result_ptr >() << res ); + ResultUrlChecker* checker = new ResultUrlChecker( origquery, nullptr, QList< result_ptr >() << res ); QEventLoop loop; connect( checker, SIGNAL( done() ), &loop, SLOT( quit() ) ); loop.exec(); diff --git a/src/libtomahawk/database/DatabaseResolver.cpp b/src/libtomahawk/database/DatabaseResolver.cpp index 8b1a9e5cb..924c40be1 100644 --- a/src/libtomahawk/database/DatabaseResolver.cpp +++ b/src/libtomahawk/database/DatabaseResolver.cpp @@ -59,7 +59,7 @@ DatabaseResolver::gotResults( const Tomahawk::QID qid, QList< Tomahawk::result_p foreach ( const Tomahawk::result_ptr& r, results ) r->setResolvedByResolver( this ); - Tomahawk::Pipeline::instance()->reportResults( qid, results ); + Tomahawk::Pipeline::instance()->reportResults( qid, this, results ); } diff --git a/src/libtomahawk/resolvers/JSResolverHelper.cpp b/src/libtomahawk/resolvers/JSResolverHelper.cpp index ef1afabba..a9557a6a0 100644 --- a/src/libtomahawk/resolvers/JSResolverHelper.cpp +++ b/src/libtomahawk/resolvers/JSResolverHelper.cpp @@ -155,7 +155,7 @@ JSResolverHelper::addTrackResults( const QVariantMap& results ) QString qid = results.value("qid").toString(); - Tomahawk::Pipeline::instance()->reportResults( qid, tracks ); + Tomahawk::Pipeline::instance()->reportResults( qid, m_resolver, tracks ); } diff --git a/src/libtomahawk/resolvers/ScriptResolver.cpp b/src/libtomahawk/resolvers/ScriptResolver.cpp index f17fb1a92..c9ff97bfe 100644 --- a/src/libtomahawk/resolvers/ScriptResolver.cpp +++ b/src/libtomahawk/resolvers/ScriptResolver.cpp @@ -331,7 +331,7 @@ ScriptResolver::handleMsg( const QByteArray& msg ) results << rp; } - Tomahawk::Pipeline::instance()->reportResults( qid, results ); + Tomahawk::Pipeline::instance()->reportResults( qid, this, results ); } else { diff --git a/src/libtomahawk/utils/ResultUrlChecker.cpp b/src/libtomahawk/utils/ResultUrlChecker.cpp index ab6edc3e3..0b605bef5 100644 --- a/src/libtomahawk/utils/ResultUrlChecker.cpp +++ b/src/libtomahawk/utils/ResultUrlChecker.cpp @@ -32,9 +32,11 @@ using namespace Tomahawk; -ResultUrlChecker::ResultUrlChecker( const query_ptr& query, const QList< result_ptr >& results ) +ResultUrlChecker::ResultUrlChecker( const query_ptr& query, void* r, + const QList< result_ptr >& results ) : QObject( 0 ) , m_query( query ) + , m_resolver( r ) , m_results( results ) { check(); diff --git a/src/libtomahawk/utils/ResultUrlChecker.h b/src/libtomahawk/utils/ResultUrlChecker.h index 543c6e9e1..64fee1770 100644 --- a/src/libtomahawk/utils/ResultUrlChecker.h +++ b/src/libtomahawk/utils/ResultUrlChecker.h @@ -33,10 +33,11 @@ class ResultUrlChecker : public QObject { Q_OBJECT public: - ResultUrlChecker( const query_ptr& query, const QList< result_ptr >& results ); + ResultUrlChecker( const query_ptr& query, void* r, const QList< result_ptr >& results ); virtual ~ResultUrlChecker(); query_ptr query() const { return m_query; } + void* resolver() const { return m_resolver; } QList< result_ptr > results() const { return m_results; } QList< result_ptr > validResults() const { return m_validResults; } @@ -49,6 +50,7 @@ private slots: private: query_ptr m_query; + void* m_resolver; QList< result_ptr > m_results; QList< result_ptr > m_validResults; QHash< NetworkReply*, Tomahawk::result_ptr > m_replies; From 9a4f1e045f594acedf477f2fe8da4533b151ea89 Mon Sep 17 00:00:00 2001 From: Anton Romanov Date: Fri, 15 May 2015 14:38:35 -0700 Subject: [PATCH 2/3] Change resultchecker to have userData field to specify arbitrary user data associated with checks --- src/libtomahawk/Pipeline.cpp | 7 ++++--- src/libtomahawk/utils/ResultUrlChecker.cpp | 4 ++-- src/libtomahawk/utils/ResultUrlChecker.h | 6 +++--- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/libtomahawk/Pipeline.cpp b/src/libtomahawk/Pipeline.cpp index abfd179c7..e4ed7f28f 100644 --- a/src/libtomahawk/Pipeline.cpp +++ b/src/libtomahawk/Pipeline.cpp @@ -430,7 +430,7 @@ Pipeline::onResultUrlCheckerDone( ) return; }*/ - decQIDState( q, reinterpret_cast( checker->resolver() ) ); + decQIDState( q, reinterpret_cast( checker->userData() ) ); } @@ -567,12 +567,13 @@ Pipeline::shunt( const query_ptr& q ) auto timeout = r->timeout(); if ( timeout == 0 ) timeout = DEFAULT_RESOLVER_TIMEOUT; - new FuncTimeout( r->timeout(), std::bind( &Pipeline::timeoutShunt, this, q, r ), this ); + + new FuncTimeout( timeout, std::bind( &Pipeline::timeoutShunt, this, q, r ), this ); } else { // we get here if we disable a resolver while a query is resolving - decQIDState(q, r); + // OR we are just out of resolvers while query is still resolving return; } diff --git a/src/libtomahawk/utils/ResultUrlChecker.cpp b/src/libtomahawk/utils/ResultUrlChecker.cpp index 0b605bef5..c57332f9b 100644 --- a/src/libtomahawk/utils/ResultUrlChecker.cpp +++ b/src/libtomahawk/utils/ResultUrlChecker.cpp @@ -32,11 +32,11 @@ using namespace Tomahawk; -ResultUrlChecker::ResultUrlChecker( const query_ptr& query, void* r, +ResultUrlChecker::ResultUrlChecker( const query_ptr& query, QObject* userData, const QList< result_ptr >& results ) : QObject( 0 ) , m_query( query ) - , m_resolver( r ) + , m_userData( userData ) , m_results( results ) { check(); diff --git a/src/libtomahawk/utils/ResultUrlChecker.h b/src/libtomahawk/utils/ResultUrlChecker.h index 64fee1770..3a90bc82f 100644 --- a/src/libtomahawk/utils/ResultUrlChecker.h +++ b/src/libtomahawk/utils/ResultUrlChecker.h @@ -33,11 +33,11 @@ class ResultUrlChecker : public QObject { Q_OBJECT public: - ResultUrlChecker( const query_ptr& query, void* r, const QList< result_ptr >& results ); + ResultUrlChecker( const query_ptr& query, QObject* userData, const QList< result_ptr >& results ); virtual ~ResultUrlChecker(); query_ptr query() const { return m_query; } - void* resolver() const { return m_resolver; } + void* userData() const { return m_userData; } QList< result_ptr > results() const { return m_results; } QList< result_ptr > validResults() const { return m_validResults; } @@ -50,7 +50,7 @@ private slots: private: query_ptr m_query; - void* m_resolver; + QObject* m_userData; QList< result_ptr > m_results; QList< result_ptr > m_validResults; QHash< NetworkReply*, Tomahawk::result_ptr > m_replies; From 1106fd7892b9635ade387d24c5f503568f5b8475 Mon Sep 17 00:00:00 2001 From: Anton Romanov Date: Thu, 21 May 2015 11:03:07 -0700 Subject: [PATCH 3/3] Fix return type in userData() in ResultUrlChecker --- src/libtomahawk/utils/ResultUrlChecker.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libtomahawk/utils/ResultUrlChecker.h b/src/libtomahawk/utils/ResultUrlChecker.h index 3a90bc82f..e9ca5418a 100644 --- a/src/libtomahawk/utils/ResultUrlChecker.h +++ b/src/libtomahawk/utils/ResultUrlChecker.h @@ -37,7 +37,7 @@ public: virtual ~ResultUrlChecker(); query_ptr query() const { return m_query; } - void* userData() const { return m_userData; } + QObject* userData() const { return m_userData; } QList< result_ptr > results() const { return m_results; } QList< result_ptr > validResults() const { return m_validResults; }