Merge lp:~alan-griffiths/mir/fix-1188451 into lp:~mir-team/mir/trunk

[Alan Griffiths]

frontend: detect and drop dead client sessions

frontend: detect and drop dead client sessions

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:752
http://jenkins.qa.ubuntu.com/job/mir-ci/747/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/932
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/814
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-raring-amd64-ci/232
        deb: http://jenkins.qa.ubuntu.com/job/mir-raring-amd64-ci/232/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/432

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Regression: The Mir server now wakes up 10 times per second, even when all clients are idle. That's bad. The reason is:

32 + io_service.post([&]
33 + {
34 + do
35 + {
36 + std::this_thread::sleep_for(std::chrono::milliseconds(100));
37 + connected_sessions->discard_disconnected();
38 + }
39 + while (!io_service.stopped());
40 + });

That has to be re-thought because polling is not an option.

An example of a well-behaved almost-idling client to test is mir_demo_client_fingerpaint when you don't move the pointer. And you can install and run powertop (from a remote login) to watch wakeups.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

If we really do need to poll, then just make sure it's not on a timer or in a loop.

Try piggy-backing the polling on some other event, like "the screen is redrawing (for some other clients) but I haven't got any communication from this client for a while".

[Alan Griffiths (alan-griffiths)]

On 18/06/13 04:33, Daniel van Vugt wrote:
> Regression: The Mir server now wakes up 10 times per second, even when all clients are idle. That's bad.

Agreed. Unfortunately I've not come up with anything better than polling
yet (but we could tie it to another event that wakes the server).

--
Alan Griffiths [phone]
Octopull Ltd http://www.octopull.co.uk/

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:755
http://jenkins.qa.ubuntu.com/job/mir-ci/768/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/965
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/847
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/6
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/6/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/448

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

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

Looks good. It would nice to have a test for this.

review: Approve

[Alan Griffiths (alan-griffiths)]

> Looks good. It would nice to have a test for this.

Yes, I'm thinking of rolling that with the "TODO need a way to report errors" resolution.

Happy to to that here or on another MP (depending upon much folks want the bug fix landed).

[Alan Griffiths (alan-griffiths)]

It always happens at EOD! This no longer seems to be working. (Will try again in the morning.)

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Sorry to say it doesn't work.

Following the test case in bug 1188451, I still get moveable leaked surfaces on the screen after a client dies. And even when I start a new client the dead clients' surface remains there, frozen and moveable.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

Having found the problem I can't see how it ever did anything that made me think it worked. (Wishes for a video of earlier testing session.)

Please try r759.

review: Abstain

[Daniel van Vugt (vanvugt)]

Since Eleni's extra error checking has landed...

/home/dan/bzr/mir/tmp.alan/src/server/frontend/session_mediator.cpp:64:39: error: declaration of ‘virtual mir::frontend::SessionMediator::~SessionMediator()’ has a different exception specifier
In file included from /home/dan/bzr/mir/tmp.alan/src/server/frontend/session_mediator.cpp:19:0:
/home/dan/bzr/mir/tmp.alan/include/server/mir/frontend/session_mediator.h:70:5: error: from previous declaration ‘virtual mir::frontend::SessionMediator::~SessionMediator() noexcept (true)’

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

> Since Eleni's extra error checking has landed...
>
> /home/dan/bzr/mir/tmp.alan/src/server/frontend/session_mediator.cpp:64:39:
> error: declaration of ‘virtual
> mir::frontend::SessionMediator::~SessionMediator()’ has a different exception
> specifier
> In file included from
> /home/dan/bzr/mir/tmp.alan/src/server/frontend/session_mediator.cpp:19:0:
> /home/dan/bzr/mir/tmp.alan/include/server/mir/frontend/session_mediator.h:70:5
> : error: from previous declaration ‘virtual
> mir::frontend::SessionMediator::~SessionMediator() noexcept (true)’

Weird, that's true (and fixed), but I didn't get a diagnostic. (g++ (Ubuntu 4.8.1-3ubuntu2) 4.8.1)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:759
http://jenkins.qa.ubuntu.com/job/mir-ci/778/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/979/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/861/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/16/console
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/456

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

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

And again:

/home/dan/bzr/mir/tmp.alan/tests/unit-tests/frontend/test_protobuf_communicator.cpp:47:7: error: looser throw specifier for ‘virtual {anonymous}::MockCommunicatorReport::~MockCommunicatorReport()’
In file included from /home/dan/bzr/mir/tmp.alan/tests/unit-tests/frontend/test_protobuf_communicator.cpp:21:0:
/home/dan/bzr/mir/tmp.alan/include/server/mir/frontend/communicator_report.h:36:13: error: overriding ‘virtual mir::frontend::CommunicatorReport::~CommunicatorReport() noexcept (true)’

Try merging in lp:mir to get the new error checking flag that just landed.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

> Try merging in lp:mir to get the new error checking flag that just landed.

That was merged in 758 (but that flag has nothing to do with this error).

What compiler are you using?

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:760
http://jenkins.qa.ubuntu.com/job/mir-ci/779/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/980/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/862
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/17/console
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/457

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

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

> > Try merging in lp:mir to get the new error checking flag that just landed.
>
> That was merged in 758 (but that flag has nothing to do with this error).
>
> What compiler are you using?

Nevermind, clang reports these

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:762
http://jenkins.qa.ubuntu.com/job/mir-ci/780/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/981/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/863
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/18/console
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/458

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:763
http://jenkins.qa.ubuntu.com/job/mir-ci/781/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/982
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/864
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/19/console

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:764
http://jenkins.qa.ubuntu.com/job/mir-ci/782/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/983
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/865
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/20
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/20/artifact/work/output/*zip*/output.zip
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/459

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:765
http://jenkins.qa.ubuntu.com/job/mir-ci/783/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/984
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/866
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/21
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/21/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Verified the bug is fixed.

1. Needs fixing: Unused protocol changes:
src/shared/protobuf/mir_protobuf_wire.proto

2. Query: void mf::ProtobufSocketCommunicator::start()
I find the structure of this loop a bit confusing. Can it be simplified?

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

> Verified the bug is fixed.
>
> 1. Needs fixing: Unused protocol changes:
> src/shared/protobuf/mir_protobuf_wire.proto

OK, that's an unrelated change.

> 2. Query: void mf::ProtobufSocketCommunicator::start()
> I find the structure of this loop a bit confusing. Can it be simplified?

Would using "break" in place of "return" help? (I don't feel that introducing a goto or a loop variable simplifies in this case.)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:767
http://jenkins.qa.ubuntu.com/job/mir-ci/787/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/989
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/874
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/25
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/25/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/463

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Approved based on reviewing the diff for r767. I'm assuming it still works well.

review: Approve

=== added file 'include/server/mir/frontend/communicator_report.h'
--- include/server/mir/frontend/communicator_report.h	1970-01-01 00:00:00 +0000
+++ include/server/mir/frontend/communicator_report.h	2013-06-21 08:43:41 +0000
@@ -0,0 +1,51 @@
+/*
+ * Copyright © 2013 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Alan Griffiths <[e-mail]
+ */
+
+#ifndef MIR_FRONTEND_COMMUNICATOR_REPORT_H_
+#define MIR_FRONTEND_COMMUNICATOR_REPORT_H_
+
+#include <stdexcept>
+
+namespace mir
+{
+namespace frontend
+{
+
+class CommunicatorReport
+{
+public:
+
+    virtual void error(std::exception const& error) = 0;
+
+protected:
+    virtual ~CommunicatorReport() = default;
+    CommunicatorReport() = default;
+    CommunicatorReport(const CommunicatorReport&) = delete;
+    CommunicatorReport& operator=(const CommunicatorReport&) = delete;
+};
+
+class NullCommunicatorReport : public CommunicatorReport
+{
+public:
+
+    void error(std::exception const& error);
+};
+}
+}
+
+#endif // MIR_FRONTEND_COMMUNICATOR_REPORT_H_

=== modified file 'include/server/mir/frontend/session_mediator.h'
--- include/server/mir/frontend/session_mediator.h	2013-05-30 03:50:54 +0000
+++ include/server/mir/frontend/session_mediator.h	2013-06-21 08:43:41 +0000
@@ -67,6 +67,8 @@
         std::shared_ptr<events::EventSink> const& event_sink,
         std::shared_ptr<ResourceCache> const& resource_cache);
 
+    ~SessionMediator() noexcept;
+
     /* Platform independent requests */
     void connect(::google::protobuf::RpcController* controller,
                  const ::mir::protobuf::ConnectParameters* request,

=== modified file 'include/test/mir_test/test_protobuf_server.h'
--- include/test/mir_test/test_protobuf_server.h	2013-04-24 05:22:20 +0000
+++ include/test/mir_test/test_protobuf_server.h	2013-06-21 08:43:41 +0000
@@ -19,29 +19,37 @@
 #ifndef MIR_TEST_TEST_PROTOBUF_SERVER_H_
 #define MIR_TEST_TEST_PROTOBUF_SERVER_H_
 
-#include "mir_test/stub_server_tool.h"
-#include "mir_test_doubles/stub_ipc_factory.h"
-#include "mir/frontend/communicator.h"
+#include <memory>
 
 namespace mir
 {
+namespace frontend
+{
+class Communicator;
+class CommunicatorReport;
+}
+
+namespace protobuf
+{
+class DisplayServer;
+}
+
 namespace test
 {
-
 struct TestProtobufServer
 {
-    TestProtobufServer(std::string socket_name,
-               const std::shared_ptr<protobuf::DisplayServer>& tool);
+    TestProtobufServer(
+        std::string const& socket_name,
+        std::shared_ptr<protobuf::DisplayServer> const& tool);
 
-    std::shared_ptr<frontend::Communicator> make_communicator(
-        const std::string& socket_name,
-        std::shared_ptr<frontend::ProtobufIpcFactory> const& factory);
+    TestProtobufServer(
+        std::string const& socket_name,
+        std::shared_ptr<protobuf::DisplayServer> const& tool,
+        std::shared_ptr<frontend::CommunicatorReport> const& report);
 
     // "Server" side
-    std::shared_ptr<doubles::StubIpcFactory> factory;
-    std::shared_ptr<frontend::Communicator> comm;
+    std::shared_ptr<frontend::Communicator> const comm;
 };
-
 }
 }
 #endif /* MIR_TEST_TEST_PROTOBUF_SERVER_H_ */

=== modified file 'src/server/frontend/make_protobuf_socket_communicator.cpp'
--- src/server/frontend/make_protobuf_socket_communicator.cpp	2013-04-25 09:48:54 +0000
+++ src/server/frontend/make_protobuf_socket_communicator.cpp	2013-06-21 08:43:41 +0000
@@ -22,6 +22,7 @@
 #include "mir/shell/session_container.h"
 #include "mir/shell/session.h"
 #include "protobuf_socket_communicator.h"
+#include "mir/frontend/communicator_report.h"
 
 namespace mf = mir::frontend;
 namespace mg = mir::graphics;
@@ -46,7 +47,8 @@
                     {
                         session->force_requests_to_complete();
                     });
-                });
+                },
+                std::make_shared<mf::NullCommunicatorReport>());
         });
 }
 

=== modified file 'src/server/frontend/protobuf_socket_communicator.cpp'
--- src/server/frontend/protobuf_socket_communicator.cpp	2013-04-25 09:48:54 +0000
+++ src/server/frontend/protobuf_socket_communicator.cpp	2013-06-21 08:43:41 +0000
@@ -20,6 +20,7 @@
 #include "protobuf_message_processor.h"
 #include "socket_session.h"
 
+#include "mir/frontend/communicator_report.h"
 #include "mir/frontend/protobuf_ipc_factory.h"
 #include "mir/protobuf/google_protobuf_guard.h"
 #include "event_pipe.h"
@@ -36,14 +37,16 @@
     std::string const& socket_file,
     std::shared_ptr<ProtobufIpcFactory> const& ipc_factory,
     int threads,
-    std::function<void()> const& force_requests_to_complete)
+    std::function<void()> const& force_requests_to_complete,
+    std::shared_ptr<CommunicatorReport> const& report)
 :   socket_file((std::remove(socket_file.c_str()), socket_file)),
     acceptor(io_service, socket_file),
     io_service_threads(threads),
     ipc_factory(ipc_factory),
     next_session_id(0),
     connected_sessions(std::make_shared<mfd::ConnectedSessions<mfd::SocketSession>>()),
-    force_requests_to_complete(force_requests_to_complete)
+    force_requests_to_complete(force_requests_to_complete),
+    report(report)
 {
     start_accept();
 }
@@ -85,7 +88,19 @@
 
 void mf::ProtobufSocketCommunicator::start()
 {
-    auto run_io_service = boost::bind(&ba::io_service::run, &io_service);
+    auto run_io_service = [&]
+    {
+        while (true)
+        try
+        {
+            io_service.run();
+            return;
+        }
+        catch (std::exception const& e)
+        {
+            report->error(e);
+        }
+    };
 
     for (auto& thread : io_service_threads)
     {
@@ -138,3 +153,7 @@
     }
     start_accept();
 }
+
+void mf::NullCommunicatorReport::error(std::exception const& /*error*/)
+{
+}

=== modified file 'src/server/frontend/protobuf_socket_communicator.h'
--- src/server/frontend/protobuf_socket_communicator.h	2013-04-25 09:48:54 +0000
+++ src/server/frontend/protobuf_socket_communicator.h	2013-06-21 08:43:41 +0000
@@ -52,6 +52,8 @@
 struct SocketSession;
 }
 
+class CommunicatorReport;
+
 class ProtobufSocketCommunicator : public Communicator
 {
 public:
@@ -61,7 +63,8 @@
         const std::string& socket_file,
         std::shared_ptr<ProtobufIpcFactory> const& ipc_factory,
         int threads,
-        std::function<void()> const& force_requests_to_complete);
+        std::function<void()> const& force_requests_to_complete,
+        std::shared_ptr<CommunicatorReport> const& report);
     ~ProtobufSocketCommunicator();
     void start();
     void stop();
@@ -79,6 +82,7 @@
     std::atomic<int> next_session_id;
     std::shared_ptr<detail::ConnectedSessions<detail::SocketSession>> const connected_sessions;
     std::function<void()> const force_requests_to_complete;
+    std::shared_ptr<CommunicatorReport> const report;
 };
 
 }

=== modified file 'src/server/frontend/session_mediator.cpp'
--- src/server/frontend/session_mediator.cpp	2013-06-12 15:36:31 +0000
+++ src/server/frontend/session_mediator.cpp	2013-06-21 08:43:41 +0000
@@ -61,6 +61,15 @@
 {
 }
 
+mf::SessionMediator::~SessionMediator() noexcept
+{
+    if (session)
+    {
+        report->session_error(session->name(), __PRETTY_FUNCTION__, "connection dropped without disconnect");
+        shell->close_session(session);
+    }
+}
+
 void mf::SessionMediator::connect(
     ::google::protobuf::RpcController*,
     const ::mir::protobuf::ConnectParameters* request,

=== modified file 'src/server/frontend/socket_session.cpp'
--- src/server/frontend/socket_session.cpp	2013-05-27 09:39:39 +0000
+++ src/server/frontend/socket_session.cpp	2013-06-21 08:43:41 +0000
@@ -27,6 +27,21 @@
 
 namespace mfd = mir::frontend::detail;
 
+mfd::SocketSession::SocketSession(
+    boost::asio::io_service& io_service,
+    int id_,
+    std::shared_ptr<ConnectedSessions<SocketSession>> const& connected_sessions) :
+    socket(io_service),
+    id_(id_),
+    connected_sessions(connected_sessions),
+    processor(std::make_shared<NullMessageProcessor>())
+{
+}
+
+mfd::SocketSession::~SocketSession() noexcept
+{
+}
+
 void mfd::SocketSession::send(std::string const& body)
 {
     const size_t size = body.size();
@@ -62,19 +77,22 @@
                     this, ba::placeholders::error));
 }
 
-void mfd::SocketSession::on_read_size(const boost::system::error_code& ec)
+void mfd::SocketSession::on_read_size(const boost::system::error_code& error)
 {
-    if (!ec)
+    if (error)
     {
-        size_t const body_size = (message_header_bytes[0] << 8) + message_header_bytes[1];
-        // Read newline delimited messages for now
-        ba::async_read(
-             socket,
-             message,
-             ba::transfer_exactly(body_size),
-             boost::bind(&mfd::SocketSession::on_new_message,
-                         this, ba::placeholders::error));
+        connected_sessions->remove(id());
+        BOOST_THROW_EXCEPTION(std::runtime_error(error.message()));
     }
+
+    size_t const body_size = (message_header_bytes[0] << 8) + message_header_bytes[1];
+    // Read newline delimited messages for now
+    ba::async_read(
+         socket,
+         message,
+         ba::transfer_exactly(body_size),
+         boost::bind(&mfd::SocketSession::on_new_message,
+                     this, ba::placeholders::error));
 }
 
 void mfd::SocketSession::on_new_message(const boost::system::error_code& error)

=== modified file 'src/server/frontend/socket_session.h'
--- src/server/frontend/socket_session.h	2013-05-27 09:39:39 +0000
+++ src/server/frontend/socket_session.h	2013-06-21 08:43:41 +0000
@@ -37,11 +37,9 @@
     SocketSession(
         boost::asio::io_service& io_service,
         int id_,
-        std::shared_ptr<ConnectedSessions<SocketSession>> const& connected_sessions) :
-        socket(io_service),
-        id_(id_),
-        connected_sessions(connected_sessions),
-        processor(std::make_shared<NullMessageProcessor>()) {}
+        std::shared_ptr<ConnectedSessions<SocketSession>> const& connected_sessions);
+
+    ~SocketSession() noexcept;
 
     int id() const { return id_; }
 

=== modified file 'tests/integration-tests/client/test_client_render.cpp'
--- tests/integration-tests/client/test_client_render.cpp	2013-06-06 08:36:17 +0000
+++ tests/integration-tests/client/test_client_render.cpp	2013-06-21 08:43:41 +0000
@@ -27,6 +27,8 @@
 #include "mir_test/stub_server_tool.h"
 #include "mir_test/test_protobuf_server.h"
 
+#include "mir/frontend/communicator.h"
+
 #include <gmock/gmock.h>
 #include <thread>
 #include <hardware/gralloc.h>

=== modified file 'tests/mir_test_doubles/test_protobuf_socket_server.cpp'
--- tests/mir_test_doubles/test_protobuf_socket_server.cpp	2013-04-25 09:48:54 +0000
+++ tests/mir_test_doubles/test_protobuf_socket_server.cpp	2013-06-21 08:43:41 +0000
@@ -17,22 +17,41 @@
  */
 
 #include "mir_test/test_protobuf_server.h"
+#include "mir_test_doubles/stub_ipc_factory.h"
+#include "mir/frontend/communicator_report.h"
 #include "src/server/frontend/protobuf_socket_communicator.h"
 
 namespace mt = mir::test;
 namespace mtd = mir::test::doubles;
 namespace mf = mir::frontend;
 
+namespace
+{
+std::shared_ptr<mf::Communicator> make_communicator(
+    std::string const& socket_name,
+    std::shared_ptr<mf::ProtobufIpcFactory> const& factory,
+    std::shared_ptr<mf::CommunicatorReport> const& report)
+{
+    return std::make_shared<mf::ProtobufSocketCommunicator>(
+        socket_name,
+        factory,
+        10,
+        []{},
+        report);
+}
+}
 
 mt::TestProtobufServer::TestProtobufServer(
-    std::string socket_name,
+    std::string const& socket_name,
     const std::shared_ptr<protobuf::DisplayServer>& tool) :
-    factory(std::make_shared<mtd::StubIpcFactory>(*tool)),
-    comm(make_communicator(socket_name, factory))
+    TestProtobufServer(socket_name, tool, std::make_shared<mf::NullCommunicatorReport>())
 {
 }
 
-std::shared_ptr<mf::Communicator> mt::TestProtobufServer::make_communicator(const std::string& socket_name, std::shared_ptr<frontend::ProtobufIpcFactory> const& factory)
+mt::TestProtobufServer::TestProtobufServer(
+    std::string const& socket_name,
+    const std::shared_ptr<protobuf::DisplayServer>& tool,
+    std::shared_ptr<frontend::CommunicatorReport> const& report) :
+    comm(make_communicator(socket_name, std::make_shared<mtd::StubIpcFactory>(*tool), report))
 {
-    return std::make_shared<mf::ProtobufSocketCommunicator>(socket_name, factory, 10, []{});
 }

=== modified file 'tests/unit-tests/client/test_client_mir_surface.cpp'
--- tests/unit-tests/client/test_client_mir_surface.cpp	2013-06-03 08:14:01 +0000
+++ tests/unit-tests/client/test_client_mir_surface.cpp	2013-06-21 08:43:41 +0000
@@ -30,6 +30,7 @@
 #include "src/client/rpc/mir_basic_rpc_channel.h"
 
 #include "mir/frontend/resource_cache.h"
+#include "mir/frontend/communicator.h"
 #include "mir/input/input_platform.h"
 #include "mir/input/input_receiver_thread.h"
 

=== modified file 'tests/unit-tests/frontend/test_protobuf_communicator.cpp'
--- tests/unit-tests/frontend/test_protobuf_communicator.cpp	2013-05-30 03:50:54 +0000
+++ tests/unit-tests/frontend/test_protobuf_communicator.cpp	2013-06-21 08:43:41 +0000
@@ -18,6 +18,7 @@
  */
 
 #include "mir/frontend/communicator.h"
+#include "mir/frontend/communicator_report.h"
 #include "mir/frontend/resource_cache.h"
 #include "src/server/frontend/protobuf_socket_communicator.h"
 
@@ -41,24 +42,43 @@
 namespace mt = mir::test;
 namespace mtd = mir::test::doubles;
 
+namespace
+{
+class MockCommunicatorReport : public mf::CommunicatorReport
+{
+public:
+
+    ~MockCommunicatorReport() noexcept {}
+
+    MOCK_METHOD1(error, void (std::exception const& error));
+};
+}
+
 struct ProtobufCommunicator : public ::testing::Test
 {
     static void SetUpTestCase()
     {
+        communicator_report = std::make_shared<MockCommunicatorReport>();
         stub_server_tool = std::make_shared<mt::StubServerTool>();
-        stub_server = std::make_shared<mt::TestProtobufServer>("./test_socket", stub_server_tool);
+        stub_server = std::make_shared<mt::TestProtobufServer>(
+            "./test_socket",
+            stub_server_tool,
+            communicator_report);
+    }
 
+    void SetUp()
+    {
+        using namespace testing;
+        EXPECT_CALL(*communicator_report, error(_)).Times(AnyNumber());
         stub_server->comm->start();
-    }
-
-    void SetUp()
-    {
         client = std::make_shared<mt::TestProtobufClient>("./test_socket", 100);
         client->connect_parameters.set_application_name(__PRETTY_FUNCTION__);
     }
 
     void TearDown()
     {
+        stub_server->comm->stop();
+        testing::Mock::VerifyAndClearExpectations(communicator_report.get());
         client.reset();
     }
 
@@ -66,15 +86,18 @@
     {
         stub_server.reset();
         stub_server_tool.reset();
+        communicator_report.reset();
     }
 
     std::shared_ptr<mt::TestProtobufClient> client;
+    static std::shared_ptr<MockCommunicatorReport> communicator_report;
     static std::shared_ptr<mt::StubServerTool> stub_server_tool;
 private:
     static std::shared_ptr<mt::TestProtobufServer> stub_server;
 };
 
 std::shared_ptr<mt::StubServerTool> ProtobufCommunicator::stub_server_tool;
+std::shared_ptr<MockCommunicatorReport> ProtobufCommunicator::communicator_report;
 std::shared_ptr<mt::TestProtobufServer> ProtobufCommunicator::stub_server;
 
 TEST_F(ProtobufCommunicator, create_surface_results_in_a_callback)
@@ -297,7 +320,44 @@
     auto comms = std::make_shared<mf::ProtobufSocketCommunicator>(
                     "./test_socket1", ipc_factory, 10,
                     std::bind(&MockForceRequests::force_requests_to_complete,
-                              &mock_force_requests));
+                              &mock_force_requests),
+                    std::make_shared<mf::NullCommunicatorReport>());
     comms->start();
     comms->stop();
 }
+
+TEST_F(ProtobufCommunicator,
+       disorderly_disconnection_handled)
+{
+    using namespace testing;
+
+    EXPECT_CALL(*client, create_surface_done()).Times(AnyNumber());
+    client->display_server.create_surface(
+        0,
+        &client->surface_parameters,
+        &client->surface,
+        google::protobuf::NewCallback(client.get(), &mt::TestProtobufClient::create_surface_done));
+
+    client->wait_for_create_surface();
+
+    std::mutex m;
+    std::condition_variable cv;
+    bool done = false;
+
+    ON_CALL(*communicator_report, error(_)).WillByDefault(Invoke([&] (std::exception const&)
+        {
+            std::unique_lock<std::mutex> lock(m);
+
+            done = true;
+            cv.notify_all();
+        }));
+
+    EXPECT_CALL(*communicator_report, error(_)).Times(1);
+
+    client.reset();
+
+    std::unique_lock<std::mutex> lock(m);
+
+    auto const deadline = std::chrono::system_clock::now() + std::chrono::seconds(1);
+    while (!done && cv.wait_until(lock, deadline) != std::cv_status::timeout);
+}

=== modified file 'tests/unit-tests/frontend/test_protobuf_reports_errors.cpp'
--- tests/unit-tests/frontend/test_protobuf_reports_errors.cpp	2013-01-21 12:05:00 +0000
+++ tests/unit-tests/frontend/test_protobuf_reports_errors.cpp	2013-06-21 08:43:41 +0000
@@ -20,6 +20,7 @@
 #include "mir/frontend/communicator.h"
 #include "mir/frontend/resource_cache.h"
 
+#include "mir_test/stub_server_tool.h"
 #include "mir_test/test_protobuf_server.h"
 #include "mir_test_doubles/stub_ipc_factory.h"
 #include "mir_test/test_protobuf_client.h"