Mir

Merge lp://qastaging/~afrantzis/mir/fix-1441620-simple-dispatch-thread-self-destruction-test into lp://qastaging/mir

  1. fix-1441620-simple-dispatch-thread-self-destruction-test
  2. Merge into development-branch
Proposed by Alexandros Frantzis
Status: Rejected
Rejected by: Alan Griffiths
Proposed branch: lp://qastaging/~afrantzis/mir/fix-1441620-simple-dispatch-thread-self-destruction-test
Merge into: lp://qastaging/mir
Diff against target: 80 lines (+22/-1)
3 files modified
include/common/mir/dispatch/simple_dispatch_thread.h (+4/-0)
src/common/dispatch/simple_dispatch_thread.cpp (+11/-1)
tests/unit-tests/dispatch/test_simple_dispatch_thread.cpp (+7/-0)
To merge this branch: bzr merge lp://qastaging/~afrantzis/mir/fix-1441620-simple-dispatch-thread-self-destruction-test
Reviewer Review Type Date Requested Status
Chris Halse Rogers Needs Information
Kevin DuBois (community) Approve
PS Jenkins bot (community) continuous-integration Needs Fixing
Alan Griffiths Approve
Alberto Aguirre (community) Approve
Review via email: mp+255686@code.qastaging.launchpad.net

Commit message

tests: Ensure the dispatch thread is finished before leaving the SimpleDispatchThread.handles_destruction_from_dispatch_callback test (LP: #1441620)

Description of the change

tests: Ensure the dispatch thread is finished before leaving the SimpleDispatchThread.handles_destruction_from_dispatch_callback test (LP: #1441620)

I've been trying to think of away around the issue, but by detaching a thread we don't have any way to synchronize with its destruction. Note that synchronizing with the end of our thread callback is not enough, since the underlying thread keeps copies of the parameters we pass to it which are released when the thread (*not* the std::thread object) is destroyed.

This MP adds a function, used only for testing, that can "steal" the thread on self destruction instead of detaching it, allowing the test to synchronize properly.

As (a very hacky) alternative we could move the handles_destruction_from_dispatch_callback away from any tests that fork(), so that the detatched thread has time to finish before valgrind checks for leaks, at least for our normal CI test runs. I am not in favor of such a solution.

To post a comment you must log in.
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

OK.

review: Approve
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

Seems OK as a bugfix. But do we really want to detach threads that own things?

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:2469
http://jenkins.qa.ubuntu.com/job/mir-ci/3483/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2001
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2000
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1951/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1480
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1480/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1951/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/3483/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

CI failure is https://bugs.launchpad.net/mir/+bug/1442020, and the MP fixing 1442020 may fail because of problem fixed by this MP. We have a circular landing dependency!

So, if we are lucky, the MP fixing 1442020 will land successfully (currently building for autolanding), otherwise it's best to merge at least one of the MPs manually to speed up the process.

Revision history for this message
Kevin DuBois (kdub) :
review: Approve
Revision history for this message
Chris Halse Rogers (raof) wrote :

Another option would be to abort instead of detaching the thread; that's implemented here: https://code.launchpad.net/~raof/mir/remove-eventloop-self-immolation/+merge/255767

Rather than passing in a reference to a promise, would it make more sense to have a method
std::shared_future<void> shutdown_notifier()
and have ~SimpleDispatchThread set_value_at_thread_exit() in the case of destruction-from-eventloop-thread?

I'm happy with either.

review: Needs Information
Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

> Another option would be to abort instead of detaching the thread; that's
> implemented here: https://code.launchpad.net/~raof/mir/remove-eventloop-self-
> immolation/+merge/255767

I was under the impression that self-destruction was a feature we needed to support (hence the test). If we don't need it, I am fine with dropping it/making it an error like you suggest.

> Rather than passing in a reference to a promise, would it make more sense to
> have a method
> std::shared_future<void> shutdown_notifier()
> and have ~SimpleDispatchThread set_value_at_thread_exit() in the case of
> destruction-from-eventloop-thread?

Unfortunately g++ 4.9 doesn't support set_*_at_thread_exit.

Revision history for this message
Chris Halse Rogers (raof) wrote :

On Fri, Apr 10, 2015 at 5:25 PM, Alexandros Frantzis
<email address hidden> wrote:
>> Another option would be to abort instead of detaching the thread;
>> that's
>> implemented here:
>> https://code.launchpad.net/~raof/mir/remove-eventloop-self-
>> immolation/+merge/255767
>
> I was under the impression that self-destruction was a feature we
> needed to support (hence the test). If we don't need it, I am fine
> with dropping it/making it an error like you suggest.

It was the simplest solution for a problem I needed solving; as it's
causing problems I could probably do something different instead...

>
>> Rather than passing in a reference to a promise, would it make more
>> sense to
>> have a method
>> std::shared_future<void> shutdown_notifier()
>> and have ~SimpleDispatchThread set_value_at_thread_exit() in the
>> case of
>> destruction-from-eventloop-thread?
>
> Unfortunately g++ 4.9 doesn't support set_*_at_thread_exit.

Sadface. I really liked the idea of shutdown_notifier().

It'd actually be pretty trivial to implement
set_value_at_(close_enough_to_)thread_exit().

Revision history for this message
Chris Halse Rogers (raof) wrote :

And, because I felt like a relaxing last half hour of week, we have https://code.launchpad.net/~raof/mir/simple-dispatch-shutdown-notifier/+merge/255782

Select whichever fix you like the best :)

Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

> Another option would be to abort instead of detaching the thread; that's
> implemented here: https://code.launchpad.net/~raof/mir/remove-eventloop-self-
> immolation/+merge/255767

Agreed

Unmerged revisions

2469. By Alexandros Frantzis

tests: Ensure the dispatch thread is finished before leaving the SimpleDispatchThread.handles_destruction_from_dispatch_callback test

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches