Merge lp:~michihenning/unity-scopes-api/fix-reaper-finished into lp:unity-scopes-api/devel

[Michi Henning]

Fixed ReplyObject::finished() to call disconnect() last. When the
reaper called finished(), the early disconnect() dropped the
last shared_ptr to this instance, causing the call to
listener_base_->finished() to go into an already-destroyed
instance. Added test for this case.

Fixed ReplyObject::finished() to call disconnect() last. When the
reaper called finished(), the early disconnect() dropped the
last shared_ptr to this instance, causing the call to
listener_base_->finished() to go into an already-destroyed
instance. Added test for this case.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:424
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/957/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-amd64-ci/511
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-armhf-ci/511
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-armhf-ci/511/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-i386-ci/509

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:426
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/960/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-amd64-ci/514
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-armhf-ci/514
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-armhf-ci/514/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-i386-ci/512

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

review: Approve (continuous-integration)

[Michal Hruby (mhr3)]

+1

[Michal Hruby (mhr3)]

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-autolanding/369/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-amd64-autolanding/193/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-armhf-autolanding/192/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-i386-autolanding/193/console

review: Needs Fixing (continuous-integration)

[lp:~michihenning/unity-scopes-api/fix-reaper-finished]

427. By Michi Henning on 2014-07-30

Fixed compilation error due to changes on devel and fixed incorrect test name.

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
Approved revid is not set in launchpad. This is most likely a launchpad issue and re-approve should fix it. There is also a chance (although a very small one) this is a permission problem of the ps-jenkins bot.
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-autolanding/370/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-amd64-autolanding/194
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-armhf-autolanding/193
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-armhf-autolanding/193/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-i386-autolanding/194

review: Needs Fixing (continuous-integration)

[Michi Henning (michihenning)]

Re-approving for Jenkins.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:427
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/997/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-amd64-ci/552/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-armhf-ci/551
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-armhf-ci/551/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-i386-ci/549

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:427
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/1000/
Executed test runs:
    None: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-amd64-ci/555/console
    None: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-armhf-ci/554/console
    None: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-i386-ci/552/console

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'include/unity/scopes/SearchListenerBase.h'
--- include/unity/scopes/SearchListenerBase.h	2014-07-28 08:51:57 +0000
+++ include/unity/scopes/SearchListenerBase.h	2014-07-30 21:36:19 +0000
@@ -19,13 +19,14 @@
 #ifndef UNITY_SCOPES_SEARCHLISTENERBASE_H
 #define UNITY_SCOPES_SEARCHLISTENERBASE_H
 
+#include <unity/scopes/Annotation.h>
+#include <unity/scopes/CategorisedResult.h>
+#include <unity/scopes/Category.h>
+#include <unity/scopes/Department.h>
+#include <unity/scopes/FilterBase.h>
 #include <unity/scopes/ListenerBase.h>
 #include <unity/util/DefinesPtrs.h>
 #include <unity/util/NonCopyable.h>
-#include <unity/scopes/Category.h>
-#include <unity/scopes/Annotation.h>
-#include <unity/scopes/Department.h>
-#include <unity/scopes/FilterBase.h>
 
 #include <string>
 
@@ -35,9 +36,6 @@
 namespace scopes
 {
 
-class CategorisedResult;
-class FilterState;
-
 namespace experimental
 {
 class Annotation;

=== modified file 'src/scopes/internal/ReplyObject.cpp'
--- src/scopes/internal/ReplyObject.cpp	2014-07-28 09:55:08 +0000
+++ src/scopes/internal/ReplyObject.cpp	2014-07-30 21:36:19 +0000
@@ -149,7 +149,6 @@
     // Only one thread can reach this point, any others were thrown out above.
 
     reap_item_->destroy();
-    disconnect();               // Disconnect self from middleware, if this hasn't happened yet.
 
     // Wait until all currently executing calls to push() have completed.
     unique_lock<mutex> lock(mutex_);
@@ -175,6 +174,12 @@
         cerr << "ReplyObject::finished(): unknown exception" << endl;
         // TODO: log error
     }
+
+    // Disconnect self from middleware, if this hasn't happened yet.
+    // We do this last because disconnect() can trigger
+    // the destructor of this instance (if finished() is called
+    // by the reaper, for example).
+    disconnect();
 }
 
 void ReplyObject::info(OperationInfo const& op_info) noexcept

=== modified file 'test/gtest/scopes/CMakeLists.txt'
--- test/gtest/scopes/CMakeLists.txt	2014-07-29 09:33:53 +0000
+++ test/gtest/scopes/CMakeLists.txt	2014-07-30 21:36:19 +0000
@@ -17,6 +17,7 @@
 add_subdirectory(RadioButtonsFilter)
 add_subdirectory(RangeInputFilter)
 add_subdirectory(RatingFilter)
+add_subdirectory(ReplyReaper)
 add_subdirectory(SwitchFilter)
 add_subdirectory(ValueSliderFilter)
 add_subdirectory(PreviewWidget)

=== added directory 'test/gtest/scopes/ReplyReaper'
=== added file 'test/gtest/scopes/ReplyReaper/CMakeLists.txt'
--- test/gtest/scopes/ReplyReaper/CMakeLists.txt	1970-01-01 00:00:00 +0000
+++ test/gtest/scopes/ReplyReaper/CMakeLists.txt	2014-07-30 21:36:19 +0000
@@ -0,0 +1,10 @@
+configure_file(Registry.ini.in ${CMAKE_CURRENT_BINARY_DIR}/Registry.ini)
+configure_file(Runtime.ini.in ${CMAKE_CURRENT_BINARY_DIR}/Runtime.ini)
+configure_file(Zmq.ini.in ${CMAKE_CURRENT_BINARY_DIR}/Zmq.ini)
+
+add_definitions(-DTEST_DIR="${CMAKE_CURRENT_BINARY_DIR}")
+
+add_executable(ReplyReaper_test ReplyReaper_test.cpp NoReplyScope.cpp)
+target_link_libraries(ReplyReaper_test ${TESTLIBS})
+
+add_test(ReplyReaper ReplyReaper_test)

=== added file 'test/gtest/scopes/ReplyReaper/NoReplyScope.cpp'
--- test/gtest/scopes/ReplyReaper/NoReplyScope.cpp	1970-01-01 00:00:00 +0000
+++ test/gtest/scopes/ReplyReaper/NoReplyScope.cpp	2014-07-30 21:36:19 +0000
@@ -0,0 +1,61 @@
+/*
+ * Copyright (C) 2014 Canonical Ltd
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Michi Henning <[e-mail]>
+ */
+
+#include "NoReplyScope.h"
+
+#include <unity/scopes/ScopeBase.h>
+
+#include <thread>
+
+#include <gtest/gtest.h>
+
+using namespace std;
+using namespace unity::scopes;
+
+namespace
+{
+
+class Query : public SearchQueryBase
+{
+public:
+    Query(CannedQuery const& query, SearchMetadata const& metadata)
+        : SearchQueryBase(query, metadata)
+    {
+    }
+
+    virtual void run(SearchReplyProxy const&) override
+    {
+        this_thread::sleep_for(chrono::seconds(3));
+    }
+
+    virtual void cancelled() override
+    {
+    }
+};
+
+}  // namespace
+
+SearchQueryBase::UPtr NoReplyScope::search(CannedQuery const& query, SearchMetadata const& metadata)
+{
+    return SearchQueryBase::UPtr(new Query(query, metadata));
+}
+
+PreviewQueryBase::UPtr NoReplyScope::preview(Result const&, ActionMetadata const&)
+{
+    return nullptr;  // Not called
+}

=== added file 'test/gtest/scopes/ReplyReaper/NoReplyScope.h'
--- test/gtest/scopes/ReplyReaper/NoReplyScope.h	1970-01-01 00:00:00 +0000
+++ test/gtest/scopes/ReplyReaper/NoReplyScope.h	2014-07-30 21:36:19 +0000
@@ -0,0 +1,33 @@
+/*
+ * Copyright (C) 2014 Canonical Ltd
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Michi Henning <[e-mail]>
+ */
+
+#ifndef TEST_NOREPLYSCOPE_H
+#define TEST_NOREPLYSCOPE_H
+
+#include <unity/scopes/ScopeBase.h>
+
+class NoReplyScope : public unity::scopes::ScopeBase
+{
+public:
+    virtual unity::scopes::SearchQueryBase::UPtr search(unity::scopes::CannedQuery const&,
+                                                        unity::scopes::SearchMetadata const&) override;
+    virtual unity::scopes::PreviewQueryBase::UPtr preview(unity::scopes::Result const&,
+                                                          unity::scopes::ActionMetadata const&) override;
+};
+
+#endif

=== added file 'test/gtest/scopes/ReplyReaper/Registry.ini.in'
--- test/gtest/scopes/ReplyReaper/Registry.ini.in	1970-01-01 00:00:00 +0000
+++ test/gtest/scopes/ReplyReaper/Registry.ini.in	2014-07-30 21:36:19 +0000
@@ -0,0 +1,6 @@
+[Registry]
+Middleware = Zmq
+Zmq.ConfigFile = @CMAKE_CURRENT_BINARY_DIR@/Zmq.ini
+Scope.InstallDir = /unused
+Click.InstallDir = /unused
+Scoperunner.Path = /unused

=== added file 'test/gtest/scopes/ReplyReaper/ReplyReaper_test.cpp'
--- test/gtest/scopes/ReplyReaper/ReplyReaper_test.cpp	1970-01-01 00:00:00 +0000
+++ test/gtest/scopes/ReplyReaper/ReplyReaper_test.cpp	2014-07-30 21:36:19 +0000
@@ -0,0 +1,107 @@
+/*
+ * Copyright (C) 2014 Canonical Ltd
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Michi Henning <[e-mail]>
+ */
+
+#include <unity/scopes/internal/RegistryObject.h>
+#include <unity/scopes/internal/RuntimeImpl.h>
+#include <unity/scopes/internal/ScopeImpl.h>
+
+#include <boost/algorithm/string.hpp>
+#include <gtest/gtest.h>
+
+#include "NoReplyScope.h"
+
+using namespace std;
+using namespace unity::scopes;
+using namespace unity::scopes::internal;
+
+class NullReceiver : public SearchListenerBase
+{
+public:
+    NullReceiver()
+        : query_complete_(false)
+    {
+    }
+
+    virtual void push(CategorisedResult /* result */) override
+    {
+    }
+
+    virtual void finished(CompletionDetails const& details) override
+    {
+        // Check that finished() was called by the reaper.
+        EXPECT_EQ(CompletionDetails::Error, details.status()) << details.message();
+        EXPECT_TRUE(boost::starts_with(details.message(), "No activity on ReplyObject for scope"));
+        lock_guard<mutex> lock(mutex_);
+        query_complete_ = true;
+        cond_.notify_all();
+    }
+
+    void wait_until_finished()
+    {
+        unique_lock<mutex> lock(mutex_);
+        cond_.wait(lock, [this] { return this->query_complete_; });
+    }
+
+private:
+    bool query_complete_;
+    mutex mutex_;
+    condition_variable cond_;
+};
+
+std::shared_ptr<core::posix::SignalTrap>
+    trap(core::posix::trap_signals_for_all_subsequent_threads({core::posix::Signal::sig_chld}));
+std::unique_ptr<core::posix::ChildProcess::DeathObserver>
+    death_observer(core::posix::ChildProcess::DeathObserver::create_once_with_signal_trap(trap));
+
+RuntimeImpl::SPtr run_test_registry()
+{
+    RuntimeImpl::SPtr runtime = RuntimeImpl::create("TestRegistry", TEST_DIR "/Runtime.ini");
+    MiddlewareBase::SPtr middleware = runtime->factory()->create("TestRegistry", "Zmq", "Zmq.ini");
+    RegistryObject::SPtr reg_obj(std::make_shared<RegistryObject>(*death_observer, std::make_shared<Executor>(), middleware));
+    middleware->add_registry_object("TestRegistry", reg_obj);
+    return runtime;
+}
+
+void scope_thread(Runtime::SPtr const& rt, string const& runtime_ini_file)
+{
+    NoReplyScope scope;
+    rt->run_scope(&scope, runtime_ini_file, "");
+}
+
+TEST(ReplyReaper, reap)
+{
+    auto reg_rt = run_test_registry();
+
+    Runtime::SPtr no_reply_rt = move(Runtime::create_scope_runtime("NoReplyScope", TEST_DIR "/Runtime.ini"));
+    std::thread scope_t(scope_thread, no_reply_rt, TEST_DIR "/Runtime.ini");
+
+    // Run a query in the scope. The query will do nothing for 3 seconds,
+    // but the reaper will reap after at most 2 seconds.
+    auto rt = internal::RuntimeImpl::create("", TEST_DIR "/Runtime.ini");
+    auto mw = rt->factory()->create("NoReplyScope", "Zmq", TEST_DIR "/Zmq.ini");
+    mw->start();
+    auto proxy = mw->create_scope_proxy("NoReplyScope");
+    auto scope = internal::ScopeImpl::create(proxy, rt.get(), "NoReplyScope");
+
+    auto receiver = make_shared<NullReceiver>();
+    scope->search("test", SearchMetadata("en", "phone"), receiver);
+    receiver->wait_until_finished();
+
+    no_reply_rt->destroy();
+    scope_t.join();
+}

=== added file 'test/gtest/scopes/ReplyReaper/Runtime.ini.in'
--- test/gtest/scopes/ReplyReaper/Runtime.ini.in	1970-01-01 00:00:00 +0000
+++ test/gtest/scopes/ReplyReaper/Runtime.ini.in	2014-07-30 21:36:19 +0000
@@ -0,0 +1,7 @@
+[Runtime]
+Registry.Identity = TestRegistry
+Registry.ConfigFile = @CMAKE_CURRENT_BINARY_DIR@/Registry.ini
+Reap.Expiry = 2
+Reap.Interval = 1
+Default.Middleware = Zmq
+Zmq.ConfigFile = @CMAKE_CURRENT_BINARY_DIR@/Zmq.ini

=== added file 'test/gtest/scopes/ReplyReaper/Zmq.ini.in'
--- test/gtest/scopes/ReplyReaper/Zmq.ini.in	1970-01-01 00:00:00 +0000
+++ test/gtest/scopes/ReplyReaper/Zmq.ini.in	2014-07-30 21:36:19 +0000
@@ -0,0 +1,2 @@
+[Zmq]
+EndpointDir = /tmp