Merge lp:~marcustomlinson/unity-scopes-api/lp-1267917-2 into lp:unity-scopes-api

[Marcus Tomlinson]

Ensure that manager_ and reply_ objects are created and destroyed from within the same Qt thread.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:130
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-ci/252/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-trusty-amd64-ci/252
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-trusty-armhf-ci/252
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-trusty-armhf-ci/252/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-trusty-i386-ci/252

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-ci/252/rebuild

review: Approve (continuous-integration)

[Michal Hruby (mhr3)]

11 + moveToThread(this);

Shouldn't be necessary.

20 + reply_->deleteLater();

The reply_ object is created inside the thread, so it should be destroyed inside the thread, isn't that what the original error was about?

review: Needs Information

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:131
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-ci/254/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-trusty-amd64-ci/254
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-trusty-armhf-ci/254/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-trusty-i386-ci/254

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-ci/254/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:132
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-ci/255/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-trusty-amd64-ci/255
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-trusty-armhf-ci/255
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-trusty-armhf-ci/255/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-trusty-i386-ci/255

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-ci/255/rebuild

review: Approve (continuous-integration)

[Michal Hruby (mhr3)]

Hmm, looks a bit odd, are we really creating a thread solely to serve one request?

Nonetheless, it does fix the issue, I'm just not convinced it's a good idea to rely on emitting a signal to quit the thread, as this is inverting the responsibility and the thread is no longer a worker, but a "I'll quit when you ask me to" thing.

[Marcus Tomlinson (marcustomlinson)]

This is a bit of a tricky situation. Consider the requirements:

1. HttpClientQt.h should not contain Qt code that requires any more than forward declarations (this is to avoid anything outside of this class having to link to Qt).

2. A call to HttpClientQt::get needs to return a handle to the future result and asynchronously execute the request.

3. QSocketNotifier (in QNetworkAccessManager) can only be used with threads started with QThread.

4. The manager_ and reply_ Qt members (regardless of who their parent class is) need to be created and destroyed in the same QThread.

In order to satisfy req.1, we include the HttpClientThread class in the HttpClientQt cpp file. This class can then contain all the Qt specific stuff like signals/slots.

req.2 is performed by spawning a C++11 worker thread on request which eventually assigns a promise with the result. This thread then uses an HttpClientQtThread object to execute the query (yes, HttpClientQtThread is not really used as a worker thread as you pointed out)

Considering we already have a C++11 worker thread per request, does the helper Qt class really need to be a QThread? Well yes. I tried this class as just a QObject with synchronous methods but ended up with errors on new-ing QNetworkAccessManager (hence req.3).

Naturally then, as we now have a QThread available, we can use this thread to create, use and destroy the manager_ and reply_ HttpClientQtThread members (req.4). Lastly, because we need to access the reply_ object after it is assigned but before it is destroyed, we need to suspend the QThread until HttpClientQtThread signals it to exit (on destruction).

I hope this makes some sense as to why the code is structured this way (By no means am I saying that this is the only possible solution).

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:133
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-ci/261/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-trusty-amd64-ci/261
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-trusty-armhf-ci/261
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-trusty-armhf-ci/261/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-trusty-i386-ci/261

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-ci/261/rebuild

review: Approve (continuous-integration)

[Michal Hruby (mhr3)]

> Naturally then, as we now have a QThread available, we can use this thread to
> create, use and destroy the manager_ and reply_ HttpClientQtThread members
> (req.4). Lastly, because we need to access the reply_ object after it is
> assigned but before it is destroyed, we need to suspend the QThread until
> HttpClientQtThread signals it to exit (on destruction).

I wouldn't agree here, all the worker is asked to do is to return whether the http request succeeded plus a memory buffer with the reply, there's absolutely no need to keep the reply_ object alive as long as these two properties are stored somewhere. And then the inversion goes away.

I still disagree with using a thread per-request, but that's outside of scope of this MP.

[Michal Hruby (mhr3)]

Why is the query_done still there? Why not just quit() the thread?

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:134
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-ci/262/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-trusty-amd64-ci/262
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-trusty-armhf-ci/262
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-trusty-armhf-ci/262/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-trusty-i386-ci/262

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-ci/262/rebuild

review: Approve (continuous-integration)

[Michal Hruby (mhr3)]

+1, let's get it in!

review: Approve

[lp:~marcustomlinson/unity-scopes-api/lp-1267917-2]

138. By Marcus Tomlinson on 2014-01-22

Create a direct connection between manager.finished() and got_reply() in order to avoid reply being freed before use.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:136
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-ci/263/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-trusty-amd64-ci/263/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-trusty-armhf-ci/263
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-trusty-armhf-ci/263/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-trusty-i386-ci/263/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-ci/263/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'include/unity/scopes/internal/smartscopes/HttpClientQtThread.h'
--- include/unity/scopes/internal/smartscopes/HttpClientQtThread.h	2014-01-13 07:18:08 +0000
+++ include/unity/scopes/internal/smartscopes/HttpClientQtThread.h	2014-01-22 11:48:09 +0000
@@ -38,12 +38,15 @@
     HttpClientQtThread(const QUrl& url, uint timeout);
     ~HttpClientQtThread();
 
+    bool get_reply(std::string& reply);
+
+private:
     void run();
-    QNetworkReply* getReply();
 
 public Q_SLOTS:
     void cancel();
-    void queryDone(QNetworkReply*);
+    void timeout();
+    void got_reply(QNetworkReply* reply);
 
 Q_SIGNALS:
     void abort();
@@ -51,9 +54,10 @@
 private:
     QUrl url_;
     uint timeout_;
-    QNetworkReply* reply_;
-    QNetworkAccessManager* manager_;
     std::mutex reply_mutex_;
+
+    bool success_;
+    std::string reply_;
 };
 
 #endif // UNITY_SCOPES_INTERNAL_SMARTSCOPES_HTTPCLIENTQTTHREAD_H

=== modified file 'src/scopes/internal/smartscopes/HttpClientQt.cpp'
--- src/scopes/internal/smartscopes/HttpClientQt.cpp	2014-01-13 12:25:13 +0000
+++ src/scopes/internal/smartscopes/HttpClientQt.cpp	2014-01-22 11:48:09 +0000
@@ -104,32 +104,18 @@
 
         get_qt_thread_->start();
         loop.exec();
-        get_qt_thread_->wait();
-
-        QNetworkReply* reply = get_qt_thread_->getReply();
-
-        if (!reply)
-        {
-            // no reply
-            unity::ResourceException e("No reply from " + request_url + ":" + std::to_string(port));
-            promise_->set_exception(e.self());
-        }
-        else if (!reply->isFinished())
-        {
-            // incomplete reply
-            unity::ResourceException e("Incomplete reply from " + request_url + ":" + std::to_string(port));
-            promise_->set_exception(e.self());
-        }
-        else if (reply->error() != QNetworkReply::NoError)
-        {
-            // communication error
-            unity::ResourceException e(reply->errorString().toStdString());
+
+        std::string reply;
+        bool success = get_qt_thread_->get_reply(reply);
+
+        if (!success)
+        {
+            unity::ResourceException e(reply);
             promise_->set_exception(e.self());
         }
         else
         {
-            QString reply_string(reply->readAll());
-            promise_->set_value(reply_string.toStdString());
+            promise_->set_value(reply);
         }
     });
 

=== modified file 'src/scopes/internal/smartscopes/HttpClientQtThread.cpp'
--- src/scopes/internal/smartscopes/HttpClientQtThread.cpp	2014-01-13 12:25:13 +0000
+++ src/scopes/internal/smartscopes/HttpClientQtThread.cpp	2014-01-22 11:48:09 +0000
@@ -27,8 +27,7 @@
     : QThread(),
       url_(url),
       timeout_(timeout),
-      reply_(nullptr),
-      manager_(nullptr)
+      success_(false)
 {
 }
 
@@ -36,48 +35,85 @@
 {
     cancel();
 
-    delete reply_;
-    delete manager_;
+    wait();
+}
+
+bool HttpClientQtThread::get_reply(std::string& reply)
+{
+    std::lock_guard<std::mutex> lock(reply_mutex_);
+
+    reply = reply_;
+    return success_;
 }
 
 void HttpClientQtThread::run()
 {
-    {
-        std::lock_guard<std::mutex> lock(reply_mutex_);
-
-        manager_ = new QNetworkAccessManager();
-
-        QNetworkRequest request(url_);
-
-        reply_ = manager_->get(request);
-
-        connect(manager_, &QNetworkAccessManager::finished, this, &HttpClientQtThread::queryDone);
-        connect(this, &HttpClientQtThread::abort, reply_, &QNetworkReply::abort);
-    }
+    QNetworkAccessManager* manager = new QNetworkAccessManager();
+
+    QNetworkRequest request(url_);
+
+    QNetworkReply* reply = manager->get(request);
+
+    connect(manager, &QNetworkAccessManager::finished, this, &HttpClientQtThread::got_reply, Qt::DirectConnection);
+    connect(this, &HttpClientQtThread::abort, reply, &QNetworkReply::abort);
 
     QTimer timeout;
-    timeout.singleShot(timeout_, this, SLOT(cancel()));
+    timeout.singleShot(timeout_, this, SLOT(timeout()));
     QThread::exec(); // enter event loop
-}
-
-QNetworkReply* HttpClientQtThread::getReply()
-{
-    std::lock_guard<std::mutex> lock(reply_mutex_);
-    return reply_;
-}
-
-void HttpClientQtThread::queryDone(QNetworkReply*)
-{
-    quit();
+
+    delete reply;
+    delete manager;
 }
 
 void HttpClientQtThread::cancel()
 {
     std::lock_guard<std::mutex> lock(reply_mutex_);
 
-    if (reply_)
-    {
-        emit abort();
+    success_ = false;
+    reply_ = "Request to " + url_.url().toStdString() + ":" + std::to_string(url_.port()) + " cancelled";
+
+    emit abort();
+    quit();
+}
+
+void HttpClientQtThread::timeout()
+{
+    std::lock_guard<std::mutex> lock(reply_mutex_);
+
+    success_ = false;
+    reply_ = "Request to " + url_.url().toStdString() + ":" + std::to_string(url_.port()) + " timed out";
+
+    emit abort();
+    quit();
+}
+
+void HttpClientQtThread::got_reply(QNetworkReply* reply)
+{
+    std::lock_guard<std::mutex> lock(reply_mutex_);
+
+    if (!reply)
+    {
+        // no reply
+        success_ = false;
+        reply_ = "No reply from " + url_.url().toStdString() + ":" + std::to_string(url_.port());
+    }
+    else if (!reply->isFinished())
+    {
+        // incomplete reply
+        success_ = false;
+        reply_ = "Incomplete reply from " + url_.url().toStdString() + ":" + std::to_string(url_.port());
+    }
+    else if (reply->error() != QNetworkReply::NoError)
+    {
+        // communication error
+        success_ = false;
+        reply_ = reply->errorString().toStdString();
+    }
+    else
+    {
+        success_ = true;
+        QByteArray byte_array = reply->readAll();
+        reply_ = std::string(byte_array.constData(), byte_array.size());
     }
 
     quit();