Merge lp:~afrantzis/mir/fix-1350207-unresponsive-clients into lp:mir

[Alexandros Frantzis]

server: Work around unresponsive clients causing the server to hang (LP: #1350207)

server: Work around unresponsive clients causing the server to hang (LP: #1350207)

This MP makes the sockets used for communicating with clients non-blocking. When the server tries to write to a socket with a full send buffer it will fail instead of blocking, resulting in either dropping the event (when sending events), or disconnecting the client (when responding to a client rpc request).

This MP also increases the socket send buffer to 64KiB (default is 16KiB) to give clients a little more breathing room for transient freezes. We can increase it even more if needed (max is 4MiB).

Note that this is only a workaround, not a final fix, which would involve making the data writes asynchronous, essentially providing some more buffering on the server side (instead of the kernel). Unfortunately, this is not easily achievable with asio in this particular scenario because:

1. Asio doesn't support sending ancillary data (e.g. file descriptors) asynchronously and in coordination with normal data.

2. There is no guaranteed ordering and atomicity of asio asynchronous writes, and messages may end up being interleaved without additional care.

More discussion is needed in this area:
* what are the criteria for dropping a client (should we decide or the shell or both?)
* application-not-responding notification to shell
* if we drop messages to clients does it make sense for the clients to continue in a possibly inconsistent state (it depends on the kind of dropped messages)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1903
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2662/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1677
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1683
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1658
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1184/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/595
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/595/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2713
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/12965

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

review: Needs Fixing (continuous-integration)

[Alexandros Frantzis (afrantzis)]

CI failure is https://bugs.launchpad.net/mir/+bug/1367353

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1903
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2663/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1678
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1684
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1659
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1185
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1185/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/596
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/596/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2714
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/12969

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

review: Approve (continuous-integration)

[Cemil Azizoglu (cemil-azizoglu)]

I haven't looked at the code in detail, but it sounds like to actually fix the problem, we need to spawn a comms thread and not block the main thread.

[Chris Halse Rogers (raof)]

No, I don't think a comms thread helps particularly; that just changes the problem to “blocked clients cause unbounded memory consumption”.

I think that properly handling this would be:
a) Mark the client as unresponsive in a way that's visible to shells
b) Register a callback for when the socket becomes writable
c) Drop any additional writes to the socket
d) When the socket becomes writable, send a full state-update message so that we know the client and server agree on what the current state is.

For now bumping the buffer size and hoping that the client didn't really need the messages we dropped seems like a non-terrible improvement to the status quo. Except...

Doesn't your new while (nread != ba::buffer_size) loop replace a wait in the kernel with a busy-wait in the server, or does transfer_exactly() not work on a non-blocking socket?

You'll certainly busy-wait until there's something to read, which seems bad.

Does anyone else find ‘ba::mutable_buffers_1 const&’ hilarious? :)

review: Needs Fixing

[Alexandros Frantzis (afrantzis)]

> Doesn't your new while (nread != ba::buffer_size) loop replace a wait in the kernel
> with a busy-wait in the server, or does transfer_exactly() not work on a
> non-blocking socket?
>
> You'll certainly busy-wait until there's something to read, which seems bad.

This is indeed a busy-wait in the server, but one that doesn't currently affect us because
the receive_msg() method is only used in this context:

if (message_receiver->available_bytes() >= body_size)
{
    on_new_message(message_receiver->receive_msg(ba::buffer(body)));
}

essentially guaranteeing that we never busy-loop. I introduced the loop to ensure we retain the blocking semantics this method had previously. It's too bad that we can't set (non-)blocking mode for reads and writes separately.

[Chris Halse Rogers (raof)]

Yeah, that seems reasonable. I'm a bit surprised that boost::transfer_exactly() doesn't do the right thing, but eh.

review: Approve

[Daniel van Vugt (vanvugt)]

Tested. Bug fixed!

review: Approve

[Daniel van Vugt (vanvugt)]

Whoops. Now that I look more closely:

(1) This looks too much like an infinite loop waiting to happen. I think "!=" needs to change to "<" for safety:
  while (nread != ba::buffer_size(buffer))
Also you could incorporate the if statement by changing to a "do .. while".

(2) This worries me too:
  "* if we drop messages to clients does it make sense for the clients to continue in a possibly inconsistent state (it depends on the kind of dropped messages)"
If a busy client gets blocked doing other things just a bit too long, there's the real risk it won't be able to recover any more (rendering will freeze and never resume).

review: Needs Fixing

[Alexandros Frantzis (afrantzis)]

> (1) This looks too much like an infinite loop waiting to happen.
> I think "!=" needs to change to "<" for safety: while (nread != ba::buffer_size(buffer))

nread can't ever become larger than ba::buffer_size(buffer), since we are descreasing the buffer size we are passing to ba::read() (ba::mutable_buffers_1{buffer + nread}).

> (2) This worries me too:
> "* if we drop messages to clients does it make sense for the clients to continue in a possibly
> inconsistent state (it depends on the kind of dropped messages)"
> If a busy client gets blocked doing other things just a bit too long, there's the real risk
> it won't be able to recover any more (rendering will freeze and never resume).

As things stand, if a client is blocked and the socket becomes full, then:

1. if the server tries to send an event and it can't the event will be dropped
2. if the server tries to reply to an RPC call (e.g. next_buffer) and it can't, it will disconnect the client

So, at least we won't get a frozen client. Of course this is far from ideal, but it's a trade-off we need to decide if we want to make: we either run the risk of dropping events/disconnecting clients if they become busy for too long, or we run the risk of a blocked client hanging the whole server. Note that we can further increase the socket send buffer to allow more breathing room for transiently frozen clients if we find it necessary.

The idea behind this MP is to provide a reasonable workaround for the issue without changing too much code and risking destabilizing the frontend for rtm. I am open to other ideas.

[Daniel van Vugt (vanvugt)]

OK, weakly "Approved". I recommend fixing still...

(1) Your logic may well be perfect. I still recommend using a less-than for safety of maintenance and readability.

(2) Yeah I agree this is the lesser evil right now. I guess land it but open a new bug for the dropped response issue immediately?

[Kevin DuBois (kdub)]

Well, this looks okay from a code perspective. It seems that we're trading the 'blocked clients can hang server' bug for 'legitimately stopped clients will get forcibly disconnected' bug. I suppose its the lesser of two evils though.

review: Approve

[Kevin DuBois (kdub)]

> (2) Yeah I agree this is the lesser evil right now. I guess land it but open a
> new bug for the dropped response issue immediately?
Sounds appropriate to do

[Alberto Aguirre (albaguirre)]

30 + while (nread != ba::buffer_size(buffer))

I would prefer a < as well...but not blocking.

+1 on submitting a bug for (2) and also for the TODO.

review: Approve

[Alexandros Frantzis (afrantzis)]

> (1) Your logic may well be perfect. I still recommend using a less-than for safety of maintenance and readability.
> I would prefer a < as well...but not blocking.

Fixed.

> (2) Yeah I agree this is the lesser evil right now. I guess land it but open a new bug for the dropped response issue immediately?

I will open a new bug as soon as this gets merged.

[Daniel van Vugt (vanvugt)]

review: Approve

[Alexandros Frantzis (afrantzis)]

> I will open a new bug as soon as this gets merged.

https://bugs.launchpad.net/mir/+bug/1370117

=== modified file 'src/server/frontend/socket_messenger.cpp'
--- src/server/frontend/socket_messenger.cpp	2014-09-10 12:50:53 +0000
+++ src/server/frontend/socket_messenger.cpp	2014-09-16 08:38:35 +0000
@@ -36,6 +36,14 @@
 mfd::SocketMessenger::SocketMessenger(std::shared_ptr<ba::local::stream_protocol::socket> const& socket)
     : socket(socket)
 {
+    // Make the socket non-blocking to avoid hanging the server when a client
+    // is unresponsive. Also increase the send buffer size to 64KiB to allow
+    // more leeway for transient client freezes.
+    // See https://bugs.launchpad.net/mir/+bug/1350207
+    // TODO: Rework the messenger to support asynchronous sends
+    socket->non_blocking(true);
+    boost::asio::socket_base::send_buffer_size option(64*1024);
+    socket->set_option(option);
 }
 
 mf::SessionCredentials mfd::SocketMessenger::creator_creds() const
@@ -143,11 +151,19 @@
     ba::mutable_buffers_1 const& buffer)
 {
     bs::error_code e;
-    boost::asio::read(
-         *socket,
-         buffer,
-         boost::asio::transfer_exactly(ba::buffer_size(buffer)),
-         e);
+    size_t nread = 0;
+
+    while (nread < ba::buffer_size(buffer))
+    {
+        nread += boost::asio::read(
+             *socket,
+             ba::mutable_buffers_1{buffer + nread},
+             e);
+
+        if (e && e != ba::error::would_block)
+            break;
+    }
+
     return e;
 }
 
 
=== modified file 'tests/acceptance-tests/CMakeLists.txt'
--- tests/acceptance-tests/CMakeLists.txt	2014-09-10 12:50:53 +0000
+++ tests/acceptance-tests/CMakeLists.txt	2014-09-16 08:38:35 +0000
@@ -46,6 +46,7 @@
   test_server_without_active_outputs.cpp
   test_client_input.cpp
   test_server_startup.cpp
+  test_unresponsive_client.cpp
   ${GENERATED_PROTOBUF_SRCS}
   ${GENERATED_PROTOBUF_HDRS}
 )
 
=== added file 'tests/acceptance-tests/test_unresponsive_client.cpp'
--- tests/acceptance-tests/test_unresponsive_client.cpp	1970-01-01 00:00:00 +0000
+++ tests/acceptance-tests/test_unresponsive_client.cpp	2014-09-16 08:38:35 +0000
@@ -0,0 +1,122 @@
+/*
+ * Copyright © 2014 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: Alexandros Frantzis <[e-mail]>
+ */
+
+#include "src/server/scene/session_container.h"
+#include "mir/scene/session.h"
+#include "mir/scene/surface.h"
+
+#include "mir_test/cross_process_action.h"
+#include "mir_test_framework/display_server_test_fixture.h"
+
+#include "mir_toolkit/mir_client_library.h"
+
+#include <gtest/gtest.h>
+
+#include <string>
+#include <thread>
+
+namespace mt = mir::test;
+namespace mtf = mir_test_framework;
+
+namespace
+{
+char const* const mir_test_socket = mtf::test_socket_file().c_str();
+}
+using UnresponsiveClient = mtf::BespokeDisplayServerTestFixture;
+
+TEST_F(UnresponsiveClient, does_not_hang_server)
+{
+    struct ServerConfig : TestingServerConfiguration
+    {
+        void on_start() override
+        {
+            std::thread(
+                [this]
+                {
+                    send_events.exec(
+                    [this]
+                    {
+                        auto const sessions = the_session_container();
+
+                        for (int i = 0; i < 1000; ++i)
+                        {
+                            sessions->for_each(
+                                [i] (std::shared_ptr<mir::scene::Session> const& session)
+                                {
+                                    session->default_surface()->resize({i + 1, i + 1});
+                                });
+                        }
+                    });
+                }).detach();
+        }
+        mt::CrossProcessAction send_events;
+    } server_config;
+
+    launch_server_process(server_config);
+
+    struct ClientConfig : TestingClientConfiguration
+    {
+        void exec()
+        {
+            MirConnection* connection = nullptr;
+            MirSurface* surface = nullptr;
+
+            connect.exec(
+                [&]
+                {
+                    connection = mir_connect_sync(mir_test_socket, __PRETTY_FUNCTION__);
+
+                    MirSurfaceParameters const request_params =
+                    {
+                        __PRETTY_FUNCTION__,
+                        100, 100,
+                        mir_pixel_format_abgr_8888,
+                        mir_buffer_usage_hardware,
+                        mir_display_output_id_invalid
+                    };
+                    surface = mir_connection_create_surface_sync(connection, &request_params);
+                });
+
+            release.exec(
+                [&]
+                {
+                    // We would normally explicitly release the surface at this
+                    // point. However, because we have been filling the server
+                    // send socket buffer, releasing the surface may cause the
+                    // server to try to write to a full socket buffer when
+                    // responding, leading the server to believe that the client
+                    // is blocked, and causing a premature client disconnection.
+                    mir_connection_release(connection);
+                });
+        }
+
+        mt::CrossProcessAction connect;
+        mt::CrossProcessAction release;
+    } client_config;
+
+    auto const client_pid = launch_client_process(client_config);
+
+    run_in_test_process([&]
+    {
+        client_config.connect();
+        kill(client_pid, SIGSTOP);
+        server_config.send_events(std::chrono::seconds{10});
+        kill(client_pid, SIGCONT);
+        client_config.release();
+    });
+}
 
=== modified file 'tests/include/mir_test/cross_process_action.h'
--- tests/include/mir_test/cross_process_action.h	2013-08-20 13:39:31 +0000
+++ tests/include/mir_test/cross_process_action.h	2014-09-16 08:38:35 +0000
@@ -23,6 +23,7 @@
 #include "mir_test_framework/cross_process_sync.h"
 
 #include <functional>
+#include <chrono>
 
 namespace mir
 {
@@ -33,7 +34,7 @@
 {
 public:
     void exec(std::function<void()> const& f);
-    void operator()();
+    void operator()(std::chrono::milliseconds timeout = std::chrono::milliseconds{-1});
 
 private:
     mir_test_framework::CrossProcessSync start_sync;
 
=== modified file 'tests/include/mir_test_framework/display_server_test_fixture.h'
--- tests/include/mir_test_framework/display_server_test_fixture.h	2014-09-10 12:50:53 +0000
+++ tests/include/mir_test_framework/display_server_test_fixture.h	2014-09-16 08:38:35 +0000
@@ -62,7 +62,7 @@
 
     void launch_server_process(TestingServerConfiguration& config);
 
-    void launch_client_process(TestingClientConfiguration& config);
+    pid_t launch_client_process(TestingClientConfiguration& config);
 
     bool shutdown_server_process();
     Result  wait_for_shutdown_server_process();
 
=== modified file 'tests/include/mir_test_framework/testing_process_manager.h'
--- tests/include/mir_test_framework/testing_process_manager.h	2014-09-10 12:50:53 +0000
+++ tests/include/mir_test_framework/testing_process_manager.h	2014-09-16 08:38:35 +0000
@@ -51,7 +51,7 @@
     ~TestingProcessManager();
 
     void launch_server_process(TestingServerConfiguration& config);
-    void launch_client_process(TestingClientConfiguration& config,
+    pid_t launch_client_process(TestingClientConfiguration& config,
                                mir::options::Option const& test_options);
 
     void tear_down_clients();
 
=== modified file 'tests/mir_test/cross_process_action.cpp'
--- tests/mir_test/cross_process_action.cpp	2013-08-20 13:39:31 +0000
+++ tests/mir_test/cross_process_action.cpp	2014-09-16 08:38:35 +0000
@@ -27,8 +27,8 @@
     finish_sync.signal_ready();
 }
 
-void mt::CrossProcessAction::operator()()
+void mt::CrossProcessAction::operator()(std::chrono::milliseconds timeout)
 {
     start_sync.signal_ready();
-    finish_sync.wait_for_signal_ready();
+    finish_sync.wait_for_signal_ready_for(timeout);
 }
 
=== modified file 'tests/mir_test_framework/display_server_test_fixture.cpp'
--- tests/mir_test_framework/display_server_test_fixture.cpp	2014-09-10 12:50:53 +0000
+++ tests/mir_test_framework/display_server_test_fixture.cpp	2014-09-16 08:38:35 +0000
@@ -60,9 +60,9 @@
     process_manager.launch_server_process(functor);
 }
 
-void BespokeDisplayServerTestFixture::launch_client_process(TestingClientConfiguration& config)
+pid_t BespokeDisplayServerTestFixture::launch_client_process(TestingClientConfiguration& config)
 {
-    process_manager.launch_client_process(config, *test_options);
+    return process_manager.launch_client_process(config, *test_options);
 }
 
 bool BespokeDisplayServerTestFixture::shutdown_server_process()
 
=== modified file 'tests/mir_test_framework/testing_process_manager.cpp'
--- tests/mir_test_framework/testing_process_manager.cpp	2014-09-10 12:50:53 +0000
+++ tests/mir_test_framework/testing_process_manager.cpp	2014-09-16 08:38:35 +0000
@@ -73,15 +73,16 @@
     }
 }
 
-void mtf::TestingProcessManager::launch_client_process(TestingClientConfiguration& config, mo::Option const& test_options)
+pid_t mtf::TestingProcessManager::launch_client_process(TestingClientConfiguration& config, mo::Option const& test_options)
 {
     if (!is_test_process)
     {
-        return; // We're not in the test process, so just return gracefully
+        return 0; // We're not in the test process, so just return gracefully
     }
 
     // We're in the test process, so make sure we started a service
-    ASSERT_TRUE(server_process_was_started);
+    if (!server_process_was_started)
+        throw std::runtime_error("Trying to launch client process, but server process has not started");
 
     pid_t pid = fork();
 
@@ -121,6 +122,8 @@
     {
         clients.push_back(std::shared_ptr<Process>(new Process(pid)));
     }
+
+    return pid;
 }
 
 void mtf::TestingProcessManager::tear_down_clients()