Mir

Merge lp://qastaging/~afrantzis/mir/fix-1317801-alt into lp://qastaging/mir

  1. fix-1317801-alt
  2. Merge into development-branch
Proposed by Alexandros Frantzis
Status: Merged
Approved by: Alexandros Frantzis
Approved revision: no longer in the source branch.
Merged at revision: 1628
Proposed branch: lp://qastaging/~afrantzis/mir/fix-1317801-alt
Merge into: lp://qastaging/mir
Diff against target: 162 lines (+73/-10)
2 files modified
src/server/compositor/buffer_queue.cpp (+10/-3)
tests/unit-tests/compositor/test_buffer_queue.cpp (+63/-7)
To merge this branch: bzr merge lp://qastaging/~afrantzis/mir/fix-1317801-alt
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Daniel van Vugt Needs Fixing
Alberto Aguirre (community) Approve
Alan Griffiths Abstain
Review via email: mp+219356@code.qastaging.launchpad.net

Commit message

compositor: Fix race in BufferQueue (LP: #1317801)

An alternative approach to fixing bug 1317801, hopefully also resolving other races involving compositor_acquire() and buffer resizing when nbuffers == 1.

Copying the std::shared_ptr<> instead of using a reference to a 'buffers' vector element ensures that acquired_buffer remains valid remain valid when new elements are added to the 'buffers' vector (i.e. in client_acquire()).

This has two side effects for the nbuffers == 1 case:

 1. There is a delay in giving the compositor the resized vector (acceptable)
 2. We may compositor_release() a buffer that is no longer used by the buffer queue, and we must be careful not to add this to the pool of free buffers (fixed in this MP).

Description of the change

compositor: Fix race in BufferQueue (LP: #1317801)

An alternative approach to fixing bug 1317801, hopefully also resolving other races involving compositor_acquire() and buffer resizing when nbuffers == 1.

Copying the std::shared_ptr<> instead of using a reference to a 'buffers' vector element ensures that acquired_buffer remains valid remain valid when new elements are added to the 'buffers' vector (i.e. in client_acquire()).

This has two side effects for the nbuffers == 1 case:

 1. There is a delay in giving the compositor the resized vector (acceptable)
 2. We may compositor_release() a buffer that is no longer used by the buffer queue, and we must be careful not to add this to the pool of free buffers (fixed in this MP).

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:1625
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1606/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/166
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/167
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/167
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/125
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/125/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/123
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/123/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/391
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/391/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1425
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/7104

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1606/rebuild

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

The code looks sane but I still don't like this:

127 +/*
128 + * This is a regression test for bug lp:1317801. This bug is a race and
129 + * very difficult to reproduce with pristine code. By carefully placing
130 + * a delay in the code, we can greatly increase the chances (100% for me)
131 + * that this test catches a regression. However these delays are not
132 + * acceptable for production use, so since the test and code in their
133 + * pristine state are highly unlikely to catch the issue, I have decided
134 + * to DISABLE the test to avoid giving a false sense of security.
135 + *
136 + * Apply the aforementioned delay, by adding
137 + * std::this_thread::sleep_for(std::chrono::milliseconds{20})
138 + * just before returning the acquired_buffer at the end of
139 + * BufferQueue::compositor_acquire().
140 + */
141 +TEST_F(BufferQueueTest, DISABLED_lp_1317801_regression_test)

A disabled test that needs edits to the production code won't detect bugs and is not going to be maintained.

review: Abstain
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

Looks good.

review: Approve
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

(1) I don't think this is a clear/strong enough guarantee:

8 - auto const& acquired_buffer = buffer_for(current_compositor_buffer, buffers);
9 + auto const acquired_buffer = buffer_for(current_compositor_buffer, buffers);

As buffer_for returns a reference, acquired_buffer is still a reference, no? Even if not then it would still be clearer and safer to explicitly construct a fresh shared_ptr while you still have the safety of the lock:
    std::shared_ptr<mg::Buffer> acquired_buffer = buffer_for(current_compositor_buffer, buffers);

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

> (1) I don't think this is a clear/strong enough guarantee:
>
> 8 - auto const& acquired_buffer =
> buffer_for(current_compositor_buffer, buffers);
> 9 + auto const acquired_buffer =
> buffer_for(current_compositor_buffer, buffers);
>
> As buffer_for returns a reference, acquired_buffer is still a reference, no?

No it isn't.

> Even if not then it would still be clearer and safer to explicitly construct a
> fresh shared_ptr while you still have the safety of the lock:
> std::shared_ptr<mg::Buffer> acquired_buffer =
> buffer_for(current_compositor_buffer, buffers);

I would have said this is unnecessarily verbose. But there's clearly at least one competent developer confused by the proposed form.

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

> A disabled test that needs edits to the production code won't detect bugs and is not going to be maintained.

I will investigate alternatives and their overhead in the near future. I don't want to delay this MP with changes that will possibly cause disagreement/discussions.

> (1) I don't think this is a clear/strong enough guarantee:

Changed to explicitly use a shared_ptr<> to resolve any potential confusion (both forms are correct).

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

> > A disabled test that needs edits to the production code won't detect bugs
> and is not going to be maintained.
>
> I will investigate alternatives and their overhead in the near future. I don't
> want to delay this MP with changes that will possibly cause
> disagreement/discussions.

NB I'm not blocking - but is that worthy of a "TODO"?

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

PASSED: Continuous integration, rev:1626
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1616/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/179
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/180
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/180
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/135
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/135/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/134
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/134/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/427
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/427/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1437
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/7140

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1616/rebuild

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

Daniel's comment has been addressed, so top approving.

> NB I'm not blocking - but is that worthy of a "TODO"?

I will file a bug for this after this MP lands.

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