Mir

Merge lp:~raof/mir/no-unbounded-multimaps into lp:mir

Proposed by Chris Halse Rogers on 2017-08-17
Status: Merged
Approved by: Alan Griffiths on 2017-08-18
Approved revision: 4231
Merged at revision: 4231
Proposed branch: lp:~raof/mir/no-unbounded-multimaps
Merge into: lp:mir
Diff against target: 19 lines (+1/-1)
1 file modified
src/server/frontend/session_mediator.cpp (+1/-1)
To merge this branch: bzr merge lp:~raof/mir/no-unbounded-multimaps
Reviewer Review Type Date Requested Status
Alan Griffiths 2017-08-17 Approve on 2017-08-18
Mir CI Bot continuous-integration Approve on 2017-08-17
Review via email: mp+329174@code.launchpad.net

Commit message

mf::SessionMediator: Remove buffer associations when removing a BufferStream.

Oops. We should really erase the association between a BufferStream and its buffers when the BufferStream is freed, not just remove the buffers from the cache…

Otherwise the stream_associated_buffers multimap will grow with each allocated BufferStream but will not shrink when they a BufferStream is released.

Description of the change

Trivial fix for threaded-ipc branch.

To post a comment you must log in.
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:4231
https://mir-jenkins.ubuntu.com/job/mir-ci/3558/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/4876
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/5091
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=artful/5080
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/5080
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/5080
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/4913
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/4913/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4913
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4913/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/4913
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/4913/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/4913
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/4913/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4913
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4913/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/4913
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/4913/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4913
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4913/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/4913
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/4913/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/3558/rebuild

review: Approve (continuous-integration)
Alan Griffiths (alan-griffiths) wrote :

Don't some of the elements get deleted? By this:

    stream_associated_buffers.erase(current);

Which means by here:

+ stream_associated_buffers.erase(associated_range.first, associated_range.second);

associated_range.first is potentially an invalid iterator.

AFAICS All of this logic amounts to:

    stream_associated_buffers.erase(stream_id);

(while stream_id is in scope)

See lp:~alan-griffiths/mir/no-unbounded-multimaps/+merge/329192

review: Needs Fixing
Chris Halse Rogers (raof) wrote :

Ah, I think you've been mislead by the whitespace change above.

stream_associated_buffers.erase(current) is called in release_buffer(). The new stream_associated_buffers.erase(associated_range.first, associated_range.second) call is in release_buffer_stream.

This could indeed be replaced by a stream_associated_buffers.erase(stream_id).

Chris Halse Rogers (raof) wrote :

(Although there will nearly always be buffers associated with a bufferstream, so hitting the race where associated_range.first is invalid would be difficult…)

lp:~raof/mir/no-unbounded-multimaps updated on 2017-08-17
4231. By Chris Halse Rogers on 2017-08-17

mf::SessionMediator: Prevent stream_associated_buffers from growing without bound.

Oops. We should really erase the association between a BufferStream and its buffers when
the BufferStream is freed, not just remove the buffers from the cache…

Chris Halse Rogers (raof) wrote :

And maybe a slightly better commit message.

Alan Griffiths (alan-griffiths) wrote :

LGTM

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/server/frontend/session_mediator.cpp'
2--- src/server/frontend/session_mediator.cpp 2017-08-16 06:40:10 +0000
3+++ src/server/frontend/session_mediator.cpp 2017-08-17 23:06:43 +0000
4@@ -581,7 +581,6 @@
5 ++match;
6 if (std::find(to_release.begin(), to_release.end(), current->second) != to_release.end())
7 {
8-
9 stream_associated_buffers.erase(current);
10 }
11 }
12@@ -1039,6 +1038,7 @@
13 {
14 buffer_cache.erase(match->second);
15 }
16+ stream_associated_buffers.erase(id);
17
18 done->Run();
19 }

Subscribers

People subscribed via source and target branches