Merge lp:~mhr3/unity-scopes-api/always-join-threads into lp:unity-scopes-api/devel

[Michal Hruby]

Always join worker threads.

Always join worker threads. Otherwise if something throws an exception, the thread is destructed without being joined, and that's not a good thing to do.

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Marcus Tomlinson (marcustomlinson)]

This is cool, although, why not encapsulate completely and make this an actual class with private members and public methods. You could just add a "core::posix::ChildProcess::DeathObserver& death_observer()" method for use in the RegistryObject constructor.

Then, while you're at it, could you please update scoperunner and smartscopesproxy with the same fix. Thanks :)

review: Needs Fixing

[Michal Hruby (mhr3)]

> This is cool, although, why not encapsulate completely and make this an actual
> class with private members and public methods. You could just add a
> "core::posix::ChildProcess::DeathObserver& death_observer()" method for use in
> the RegistryObject constructor.

It's not generic enough, that's why I didn't encapsulate it more - for example the sigchild trap + deathobserver is only applicable to scope registry, nothing else.

> Then, while you're at it, could you please update scoperunner and
> smartscopesproxy with the same fix. Thanks :)

scoperunner is setup differently and doesn't have the issue, added to smartscopesproxy though.

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Marcus Tomlinson (marcustomlinson)]

Looks good. Thanks!

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/157/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-trusty-amd64-autolanding/157/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-trusty-armhf-autolanding/157
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-trusty-armhf-autolanding/157/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-trusty-i386-autolanding/157

review: Needs Fixing (continuous-integration)

[Michal Hruby (mhr3)]

Pulled the failure from s-jenskins (getting 404 from the public one):

29: Test command: /tmp/buildd/unity-scopes-api-0.4.2+14.04.20140404.2bzr302pkg0trusty157/obj-x86_64-linux-gnu/test/gtest/scopes/Registry/Registry_test
29: Test timeout computed to be: 1500
29: scoperegistry [Registry test]: unity::ResourceException: cannot open scope installation directory "/unused": No such file or directory
29: scoperegistry [Registry test]: could not open OEM installation directory, ignoring OEM scopes
29: scoperegistry [Registry test]: unity::ResourceException: cannot open scope installation directory "/tmp/buildd/.local/share/unity-scopes/": No such file or directory
29: scoperegistry [Registry test]: could not open Click installation directory, ignoring Click scopes
29: scoperegistry [Registry test]: no remote registry configured, only local scopes will be available
29: RegistryObject::ScopeProcess::exec(): Process for scope: "testscopeA" started
29: [==========] Running 1 test from 1 test case.
29: [----------] Global test environment set-up.
29: [----------] 1 test from Registry
29: [ RUN ] Registry.metadata
29: unknown file: Failure
29: C++ exception with description "unity::scopes::TimeoutException: Request timed out after 300 milliseconds" thrown in the test body.
29: [ FAILED ] Registry.metadata (724 ms)
29: [----------] 1 test from Registry (724 ms total)
29:
29: [----------] Global test environment tear-down
29: [==========] 1 test from 1 test case ran. (724 ms total)
29: [ PASSED ] 0 tests.
29: [ FAILED ] 1 test, listed below:
29: [ FAILED ] Registry.metadata

I thought this was supposed to be fixed by that awful hack?

[Marcus Tomlinson (marcustomlinson)]

Ah right. Could you please make the following change in this branch:
In scoperegistry we need to run:

"middleware->add_state_receiver_object("StateReceiver", registry->state_receiver());"

BEFORE

"middleware->add_registry_object(runtime->registry_identity(), registry);"

This way we ensure that the state receiver is ready to process requests as soon as the registry appears on the middleware.

review: Needs Fixing

[Marcus Tomlinson (marcustomlinson)]

Those lines (mentioned above) are already there, we just need then to be swapped.
Thanks!

[lp:~mhr3/unity-scopes-api/always-join-threads]

303. By Marcus Tomlinson on 2014-04-09

Init state receiver first

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Marcus Tomlinson (marcustomlinson)]

That should do it.

review: Approve

=== modified file 'scoperegistry/scoperegistry.cpp'
--- scoperegistry/scoperegistry.cpp	2014-04-04 09:40:40 +0000
+++ scoperegistry/scoperegistry.cpp	2014-04-09 16:25:44 +0000
@@ -70,6 +70,58 @@
     return path;
 }
 
+// throwing an exception without joining with a thread is a bad, bad thing to do, so let's RAII to avoid doing it
+struct SignalThreadWrapper
+{
+    std::shared_ptr<core::posix::SignalTrap> termination_trap;
+    std::shared_ptr<core::posix::SignalTrap> child_trap;
+    std::unique_ptr<core::posix::ChildProcess::DeathObserver> death_observer;
+    std::thread termination_trap_worker;
+    std::thread child_trap_worker;
+
+    SignalThreadWrapper() :
+        // We shutdown the runtime whenever these signals happen.
+        termination_trap(core::posix::trap_signals_for_all_subsequent_threads(
+            {
+                core::posix::Signal::sig_int,
+                core::posix::Signal::sig_hup,
+                core::posix::Signal::sig_term
+            })),
+        // And we maintain our list of processes with the help of sig_chld.
+        child_trap(core::posix::trap_signals_for_all_subsequent_threads(
+            {
+                core::posix::Signal::sig_chld
+            })),
+        // The death observer is required to make sure that we reap all child processes
+        // whenever multiple sigchld's are compressed together.
+        death_observer(core::posix::ChildProcess::DeathObserver::create_once_with_signal_trap(child_trap)),
+        // Starting up both traps.
+        termination_trap_worker([&]() { termination_trap->run(); }),
+        child_trap_worker([&]() { child_trap->run(); })
+    {
+    }
+
+    core::Signal<core::posix::Signal>& signal_raised()
+    {
+        return termination_trap->signal_raised();
+    }
+
+    ~SignalThreadWrapper()
+    {
+        // Stop termination_trap
+        termination_trap->stop();
+        if (termination_trap_worker.joinable())
+            termination_trap_worker.join();
+
+        // Please note that the child_trap must only be stopped once the
+        // termination_trap thread has been joined. We otherwise will encounter
+        // a race between the middleware shutting down and not receiving sigchld anymore.
+        child_trap->stop();
+        if (child_trap_worker.joinable())
+            child_trap_worker.join();
+    }
+};
+
 // Return a map of <scope, config_file> pairs for all scopes (Canonical and OEM scopes).
 // If a Canonical scope is overrideable and the OEM has configured a scope with the
 // same id, the OEM scope overrides the Canonical one.
@@ -310,29 +362,7 @@
 
     try
     {
-        // We shutdown the runtime whenever these signals happen.
-        auto termination_trap = core::posix::trap_signals_for_all_subsequent_threads(
-        {
-            core::posix::Signal::sig_int,
-            core::posix::Signal::sig_hup,
-            core::posix::Signal::sig_term
-        });
-
-        // And we maintain our list of processes with the help of sig_chld.
-        auto child_trap = core::posix::trap_signals_for_all_subsequent_threads(
-        {
-            core::posix::Signal::sig_chld
-        });
-
-        // The death observer is required to make sure that we reap all child processes
-        // whenever multiple sigchld's are compressed together.
-        auto death_observer =
-                core::posix::ChildProcess::DeathObserver::create_once_with_signal_trap(
-                    child_trap);
-
-        // Starting up both traps.
-        std::thread termination_trap_worker([termination_trap]() { termination_trap->run(); });
-        std::thread child_trap_worker([child_trap]() { child_trap->run(); });
+        SignalThreadWrapper signal_handler_wrapper;
 
         // And finally creating our runtime.
         RuntimeConfig rt_config(config_file);
@@ -368,7 +398,7 @@
 
         // Inform the signal thread that it should shutdown the middleware
         // if we get a termination signal.
-        termination_trap->signal_raised().connect([middleware](core::posix::Signal signal)
+        signal_handler_wrapper.signal_raised().connect([middleware](core::posix::Signal signal)
         {
             switch(signal)
             {
@@ -383,7 +413,7 @@
         });
 
         // The registry object stores the local and remote scopes
-        RegistryObject::SPtr registry(new RegistryObject(*death_observer));
+        RegistryObject::SPtr registry(new RegistryObject(*signal_handler_wrapper.death_observer));
 
         // Add the metadata for each scope to the lookup table.
         // We do this before starting any of the scopes, so aggregating scopes don't get a lookup failure if
@@ -414,14 +444,14 @@
             load_remote_scopes(registry, middleware, ss_reg_id, ss_reg_endpoint);
         }
 
+        // Let's add the registry's state receiver to the middleware so that scopes can inform
+        // the registry of state changes.
+        middleware->add_state_receiver_object("StateReceiver", registry->state_receiver());
+
         // Now that the registry table is populated, we can add the registry to the middleware, so
         // it starts processing incoming requests.
         middleware->add_registry_object(runtime->registry_identity(), registry);
 
-        // We also add the registry's state receiver to the middleware so that scopes can inform
-        // the registry of state changes.
-        middleware->add_state_receiver_object("StateReceiver", registry->state_receiver());
-
         // FIXME, HACK HACK HACK HACK
         // The middleware should spawn scope processes with lookup() on demand.
         // Because it currently does not have the plumbing, we start every scope immediately.
@@ -452,18 +482,6 @@
         // Wait until we are done, which happens if we receive a termination signal.
         middleware->wait_for_shutdown();
 
-        // Stop termination_trap
-        termination_trap->stop();
-        if (termination_trap_worker.joinable())
-            termination_trap_worker.join();
-
-        // Please note that the child_trap must only be stopped once the
-        // termination_trap thread has been joined. We otherwise will encounter
-        // a race between the middleware shutting down and not receiving sigchld anymore.
-        child_trap->stop();
-        if (child_trap_worker.joinable())
-            child_trap_worker.join();
-
         exit_status = 0;
     }
     catch (std::exception const& e)

=== modified file 'smartscopesproxy/smartscopesproxy.cpp'
--- smartscopesproxy/smartscopesproxy.cpp	2014-03-24 10:19:35 +0000
+++ smartscopesproxy/smartscopesproxy.cpp	2014-04-09 16:25:44 +0000
@@ -41,6 +41,35 @@
     std::cerr << "smartscopesproxy: " << msg << std::endl;
 }
 
+struct SignalThreadWrapper
+{
+    std::shared_ptr<core::posix::SignalTrap> termination_trap;
+    std::thread termination_trap_worker;
+
+    SignalThreadWrapper() :
+        termination_trap(core::posix::trap_signals_for_all_subsequent_threads(
+            {
+                core::posix::Signal::sig_int,
+                core::posix::Signal::sig_hup,
+                core::posix::Signal::sig_term
+            })),
+        termination_trap_worker([&]() { termination_trap->run(); })
+    {
+    }
+
+    core::Signal<core::posix::Signal>& signal_raised()
+    {
+        return termination_trap->signal_raised();
+    }
+
+    ~SignalThreadWrapper()
+    {
+        termination_trap->stop();
+        if (termination_trap_worker.joinable())
+            termination_trap_worker.join();
+    }
+};
+
 int main(int argc, char* argv[])
 {
     if (argc > 1 && (std::string("-h") == argv[1] || std::string("--help") == argv[1]))
@@ -76,12 +105,7 @@
 
     try
     {
-        auto trap = core::posix::trap_signals_for_all_subsequent_threads(
-        {
-            core::posix::Signal::sig_int,
-            core::posix::Signal::sig_hup,
-            core::posix::Signal::sig_term
-        });
+        SignalThreadWrapper signal_handler;
 
         ///! TODO: get these from config
         std::string ss_reg_id = "SSRegistry";
@@ -102,14 +126,12 @@
         MiddlewareBase::SPtr reg_mw = reg_rt->factory()->find(ss_reg_id, mw_kind);
         MiddlewareBase::SPtr scope_mw = scope_rt->factory()->create(ss_scope_id, mw_kind, mw_configfile);
 
-        trap->signal_raised().connect([reg_mw, scope_mw](core::posix::Signal)
+        signal_handler.signal_raised().connect([reg_mw, scope_mw](core::posix::Signal)
         {
             scope_mw->stop();
             reg_mw->stop();
         });
 
-        std::thread trap_worker([trap]() { trap->run(); });
-
         // Instantiate a SS registry object
         SSRegistryObject::SPtr reg(new SSRegistryObject(reg_mw, scope_mw->get_scope_endpoint(),
                                                         http_reply_timeout, ss_reg_refresh_rate));
@@ -131,11 +153,6 @@
         scope_mw->wait_for_shutdown();
         reg_mw->wait_for_shutdown();
 
-        trap->stop();
-
-        if (trap_worker.joinable())
-            trap_worker.join();
-
         exit_status = 0;
     }
     catch (std::exception const& e)