Mir

Merge lp:~alan-griffiths/mir/fix-1555620 into lp:mir

fix-1555620
Merge into development-branch

Proposed by Alan Griffiths on 2016-03-10

Status:

Merged

Approved by:

Alan Griffiths on 2016-03-11

Approved revision:

no longer in the source branch.

Merged at revision:

3385

Proposed branch:

lp:~alan-griffiths/mir/fix-1555620

Merge into:

lp:mir

Diff against target:

69 lines (+20/-18)

2 files modified

src/server/scene/application_session.cpp (+18/-18)
src/server/scene/application_session.h (+2/-0)

To merge this branch:

bzr merge lp:~alan-griffiths/mir/fix-1555620

Medium

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Mir CI Bot	continuous-integration		Approve on 2016-03-11
Cemil Azizoglu (community)			Approve on 2016-03-10
Kevin DuBois (community)			Approve on 2016-03-10
Alberto Aguirre (community)		2016-03-10	Approve on 2016-03-10
Review via email: mp+288663@code.launchpad.net

Commit message

scene: ensure that the buffer stream associated with a closing surface has no outstanding buffer requests

Description of the change

scene: ensure that the buffer stream associated with a closing surface has no outstanding buffer requests

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2016-03-10:

Would be good to have a test.

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2016-03-10:

LGTM.

review: Approve

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-03-10:

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-03-10:

> Would be good to have a test.

We used to have mir-mediumtest-runner.sh that ran all the clients via:

$ mir_demo_server --test-client <client>

I'm following up on why that either got lost, or now fails to detect the failure. (I suspect a blip in the transition to jenkaas - a good excuse for me to learn more.)

I'm tempted to write "mir_demo_client_try_to_crash_the_server" and plug that into that test suite.

Revision history for this message

Kevin DuBois (kdub) wrote on 2016-03-10:

lgtm.
force_requests_to_complete() is slated for removal soon, so I don't mind the lack of test.

review: Approve

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2016-03-10:

LGTM

review: Approve

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-03-10:

review: Needs Fixing (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-03-11:

> lgtm.
> force_requests_to_complete() is slated for removal soon, so I don't mind the
> lack of test.

Surely that should means a test is important - to ensure removing the call that's needed here doesn't cause a regression?

But I won't block as I do plan to improve our testing - just not in this MP.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-03-11:

Failure is lp:1552065

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-03-11:

review: Needs Fixing (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-03-11:

10:47:50 E: apt-get update failed

re-re-approving

Revision history for this message

Mir CI Bot (mir-ci-bot) on 2016-03-11:

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Alan Griffiths

Emanuele Antonio Faraone

Gerry Boland

Mir development team

 === modified file 'src/server/scene/application_session.cpp'
 --- src/server/scene/application_session.cpp	2016-03-02 08:43:21 +0000
 +++ src/server/scene/application_session.cpp	2016-03-10 15:33:39 +0000
@@ -242,17 +242,8 @@
  void ms::ApplicationSession::destroy_surface(mf::SurfaceId id)
+ {
      std::unique_lock<std::mutex> lock(surfaces_and_streams_mutex);
--    auto p = checked_find(id);
--    auto const surface = p->second;
--    session_listener->destroying_surface(*this, surface);
--    surfaces.erase(p);
--    auto stream_it = streams.find(mf::BufferStreamId(id.as_value()));
--    if (stream_it != streams.end())
--        streams.erase(stream_it);
--
--    lock.unlock();
--
--    surface_stack->remove_surface(surface);
++
++    destroy_surface(lock, checked_find(id));
+ }
  std::string ms::ApplicationSession::name() const
@@ -394,16 +385,25 @@
      if (p == surfaces.end())
          BOOST_THROW_EXCEPTION(std::runtime_error("Invalid Surface"));
--    auto const id = p->first;
--
--    session_listener->destroying_surface(*this, ss);
--    surfaces.erase(p);
--    auto stream_it = streams.find(mf::BufferStreamId(id.as_value()));
++    destroy_surface(lock, p);
++}
++
++void ms::ApplicationSession::destroy_surface(std::unique_lock<std::mutex>& lock, Surfaces::const_iterator in_surfaces)
++{
++    auto const surface = in_surfaces->second;
++    auto const id = in_surfaces->first;
++
++    session_listener->destroying_surface(*this, surface);
++    surfaces.erase(in_surfaces);
++
++    auto stream_it = streams.find(mir::frontend::BufferStreamId(id.as_value()));
      if (stream_it != streams.end())
++    {
++        stream_it->second->force_requests_to_complete();
          streams.erase(stream_it);
++    }
      lock.unlock();
--    surface_stack->remove_surface(ss);
--
++    surface_stack->remove_surface(surface);
+ }
 === modified file 'src/server/scene/application_session.h'
 --- src/server/scene/application_session.h	2016-01-29 08:18:22 +0000
 +++ src/server/scene/application_session.h	2016-03-10 15:33:39 +0000
@@ -125,6 +125,8 @@
      std::mutex mutable surfaces_and_streams_mutex;
      Surfaces surfaces;
      Streams streams;
++
++    void destroy_surface(std::unique_lock<std::mutex>& lock, Surfaces::const_iterator in_surfaces);
  };
+ }