unity-scopes-api

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

fix-settings-thread
Merge into devel

Proposed by Paweł Stołowski on 2015-05-06

Status:

Merged

Approved by:

Michi Henning on 2015-05-13

Approved revision:

585

Merged at revision:

586

Proposed branch:

lp://staging/~stolowski/unity-scopes-api/fix-settings-thread

Merge into:

lp://staging/unity-scopes-api/devel

Diff against target:

14 lines (+4/-0)

1 file modified

src/scopes/internal/SettingsDB.cpp (+4/-0)

To merge this branch:

bzr merge lp://staging/~stolowski/unity-scopes-api/fix-settings-thread

Undecided

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Michi Henning (community)		2015-05-06	Approve on 2015-05-13
PS Jenkins bot (community)	continuous-integration		Approve on 2015-05-08
Review via email: mp+258365@code.staging.launchpad.net

Commit message

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

Description of the change

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

Revision history for this message

Michi Henning (michihenning) wrote on 2015-05-06:

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

Revision history for this message

Michi Henning (michihenning) wrote on 2015-05-06:

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.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-05-06:

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)

Revision history for this message

Paweł Stołowski (stolowski) wrote on 2015-05-06:

Download full text (3.4 KiB)

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...

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/ObjectAdapter.cpp:680
#18 0x00007f7dc1a2ae30 in ?? () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#19 0x00007f7dc1eaf6aa in start_thread (arg=0x7f7d8effd700) at pthread_create.c:333
---Type <return> to continue, or q <return> to quit---
#20 0x00007f7dc1496eed in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
(gdb)

Revision history for this message

Michi Henning (michihenning) wrote on 2015-05-07:

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() :-(

Revision history for this message

Michi Henning (michihenning) wrote on 2015-05-08:

Thanks for persisting with this!

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-05-08:

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