Merge lp:~stolowski/unity-scopes-api/fix-settings-thread into lp:unity-scopes-api/devel

[Paweł Stołowski]

Join watcher thread in SettingsDb before re-creating it. This fixes crash on 'terminate called without..' exception.

Join watcher thread in SettingsDb before re-creating it. This fixes crash on 'terminate called without..' exception.

[Michi Henning (michihenning)]

I'm not clear how a race could happen between the destructor and another method. If something accesses the instance beyond its life time, I would think the problem is in the calling code?

review: Needs Information

[Michi Henning (michihenning)]

Also, interestingly, we have almost identical code in scoperegistry/DirWatcher.cpp. But there, the reset of the thread state does *not* happen.

The fix you propose may well be right, but I'd like to understand how this race exactly comes about first.

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Paweł Stołowski (stolowski)]

Ok, I got a stacktrace that I think confirms it's the line that re-creates the thread without joining when state is Idle.

#2 0x00007f7dc19d106d in __gnu_cxx::__verbose_terminate_handler() () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#3 0x00007f7dc19ceee6 in ?? () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#4 0x00007f7dc19cef31 in std::terminate() () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#5 0x00007f7dc2897432 in operator= (__t=<unknown type in /usr/lib/debug/.build-id/02/3508f849eafb29d53f113154c68b2790b4ec2f.debug, CU 0x15b4380, DIE 0x15e2e9e>, this=0x1de1138)
    at /usr/include/c++/4.9/thread:154
#6 unity::scopes::internal::SettingsDB::process_all_docs (this=this@entry=0x1de1000) at /build/buildd/unity-scopes-api-0.6.16+15.04.20150410.3/src/scopes/internal/SettingsDB.cpp:352
#7 0x00007f7dc2898c54 in unity::scopes::internal::SettingsDB::settings (this=0x1de1000) at /build/buildd/unity-scopes-api-0.6.16+15.04.20150410.3/src/scopes/internal/SettingsDB.cpp:409
#8 0x00007f7dc283587e in unity::scopes::internal::QueryBaseImpl::settings (this=0x7f7d9c006250) at /build/buildd/unity-scopes-api-0.6.16+15.04.20150410.3/src/scopes/internal/QueryBaseImpl.cpp:45
#9 0x00007f7dc28d503d in unity::scopes::QueryBase::settings (this=<optimized out>) at /build/buildd/unity-scopes-api-0.6.16+15.04.20150410.3/src/scopes/QueryBase.cpp:57
#10 0x00007f7db5390a18 in scope::Query::run(std::shared_ptr<unity::scopes::SearchReply> const&) () from /home/vagrant/spiege/src/libusername.spiegel-scope_spiegel-scope.so
#11 0x00007f7dc28397ba in unity::scopes::internal::QueryObject::run (this=this@entry=0x7f7d9c005de0, reply=std::shared_ptr (count 3, weak 0) 0x7f7d7400b060, info=...)
    at /build/buildd/unity-scopes-api-0.6.16+15.04.20150410.3/src/scopes/internal/QueryObject.cpp:123
#12 0x00007f7dc2792ab7 in unity::scopes::internal::zmq_middleware::QueryI::run_ (this=<optimized out>, current=..., in_params=...)
    at /build/buildd/unity-scopes-api-0.6.16+15.04.20150410.3/src/scopes/internal/zmq_middleware/QueryI.cpp:76
#13 0x00007f7dc279e292 in operator() (__args#2=..., __args#1=..., __args#0=..., this=0x7f7d8effc510) at /usr/include/c++/4.9/functional:2439
#14 unity::scopes::internal::zmq_middleware::ServantBase::dispatch_ (this=this@entry=0x7f7d9c005c70, current=..., in_params=..., r=...)
    at /build/buildd/unity-scopes-api-0.6.16+15.04.20150410.3/src/scopes/internal/zmq_middleware/ServantBase.cpp:70
#15 0x00007f7dc279e380 in unity::scopes::internal::zmq_middleware::ServantBase::safe_dispatch_ (this=this@entry=0x7f7d9c005c70, current=..., in_params=..., r=...)
    at /build/buildd/unity-scopes-api-0.6.16+15.04.20150410.3/src/scopes/internal/zmq_middleware/ServantBase.cpp:84
#16 0x00007f7dc278e3a9 in unity::scopes::internal::zmq_middleware::ObjectAdapter::dispatch (this=this@entry=0x7f7d9c00a0f0, pump=..., client_address="")
    at /build/buildd/unity-scopes-api-0.6.16+15.04.20150410.3/src/scopes/internal/zmq_middleware/ObjectAdapter.cpp:817
#17 0x00007f7dc278f300 in unity::scopes::internal::zmq_middleware::ObjectAdapter::worker (this=0x7f7d9c00a0f0)
    at /build/buildd/unity-scopes-api-0.6.16+15.04.20150410.3/src/scopes/internal/zmq_middleware/Obje...

[Michi Henning (michihenning)]

I'm still not convinced that this is right. In process_all_docs(), we have:

            if (thread_state_ == Idle)
            {
                thread_state_ = Running;
                thread_ = thread(&SettingsDB::watch_thread, this);
            }

If the settings db is deleted, the watch thread gets IN_DELETE_SELF. In that case, the watch thread does:

                if (event->mask & IN_DELETE_SELF)
                {
                    lock_guard<mutex> lock(mutex_);
                    state_changed_ = false;
                    watch_.dealloc();
                    watch_.reset(-1);
                    thread_state_ = Stopping;
                }
and:
            // Break from the loop if we are stopping
            {
                lock_guard<mutex> lock(mutex_);
                if (thread_state_ == Stopping)
                {
                    break;
                }
            }

This leaves the thread_state_ as Stopping. But that then would prevent the watch thread from being re-created because, in process_all_docs(), it is created only if the state is Idle. Even if it were Idle, the assignment to the thread would make it impossible to join with the previous thread.

I'm still not clear exactly what sequence of events is actually happening here when the scope calls settings() :-(

[Michi Henning (michihenning)]

Thanks for persisting with this!

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Michi Henning (michihenning)]

Yes, good to go, thanks!

review: Approve

=== modified file 'src/scopes/internal/SettingsDB.cpp'
--- src/scopes/internal/SettingsDB.cpp	2015-03-02 03:59:01 +0000
+++ src/scopes/internal/SettingsDB.cpp	2015-05-08 08:31:26 +0000
@@ -348,6 +348,10 @@
 
             if (thread_state_ == Idle)
             {
+                if (thread_.joinable()) {
+                    thread_.join();
+                }
+
                 thread_state_ = Running;
                 thread_ = thread(&SettingsDB::watch_thread, this);
             }