Merge lp:~vanvugt/mir/fix-1226139 into lp:mir

[Robert Ancell]

Stop asio::write from throwing exceptions (SIGPIPE). It will only do so
after we've closed the socket, at which point we don't really care.
(LP: #1226139)

I'm not convinced this is the right answer long term. Long term we should be ensuring that all messages are sent before the socket is closed. But that kind of race will take much longer to solve. And right now we need a quick fix to stop the server crashing on client disconnect. Particularly if this is one of the crashes holding up touch...

[Robert Ancell (robert-ancell)]

+1 on the long term needs a better solution. It would be nice if at least we logged something to stderr so we know if this occurs though since a future bug might be hidden by this change. Otherwise looks good.

review: Approve

[Alan Griffiths (alan-griffiths)]

I'm not entirely convinced by the assertion that the existing code will only throw after we've closed the socket. It would probably better to allow the exception to propagate to a point that can clear down the client session (including the connection).

OTOH not handling the exception is a very bad thing and this is an improvement.

review: Abstain

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1072
http://jenkins.qa.ubuntu.com/job/mir-ci/1539/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2084
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1969
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/777/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-armhf-ci/53
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-armhf-ci/53/artifact/work/output/*zip*/output.zip

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

review: Needs Fixing (continuous-integration)

[Robert Carr (robertcarr)]

I've lost quite a bit of time in the past to an 'exception blackhole' in frontend, so I'm a little worried about that, or just that we will lose track of it.

Let's report the exception at least, proposed a merge in to this branch:
https://code.launchpad.net/~robertcarr/mir/socket-messenger-reporting/+merge/187023

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

According to today's process this should be resubmitted targeting ~mir-team/mir/development-branch.

(It isn't an ABI break, but routing through development-branch avoids missteps.)

review: Needs Resubmitting

[Robert Ancell (robert-ancell)]

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1072
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~vanvugt/mir/fix-1226139/+merge/187649/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/30/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2141/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2026/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/30/console

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1072
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/33/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2144/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2029/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/33/console

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

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

8 +<<<<<<< TREE
9 boost::system::error_code err;
10 ba::write(*socket, ba::buffer(whole_message), err);
11 +=======
12 + try
13 + {
14 + ba::write(*socket, ba::buffer(whole_message));
15 + }
16 + catch (std::exception &)
17 + {
18 + // Don't care
19 + }
20 +>>>>>>> MERGE-SOURCE

I prefer "tree"

[Alan Griffiths (alan-griffiths)]

8 +<<<<<<< TREE
9 boost::system::error_code err;
10 ba::write(*socket, ba::buffer(whole_message), err);
11 +=======
12 + try
13 + {
14 + ba::write(*socket, ba::buffer(whole_message));
15 + }
16 + catch (std::exception &)
17 + {
18 + // Don't care
19 + }
20 +>>>>>>> MERGE-SOURCE

I prefer "tree"

review: Needs Fixing

[Robert Carr (robertcarr)]

I got branch confusion at: https://code.launchpad.net/~robertcarr/mir/socket-messenger-reporting/+merge/187822 so lets merge socket-messenger-reporting directly instead?

[lp:~vanvugt/mir/fix-1226139]

1073. By Daniel van Vugt on 2013-09-27

Merge latest dev branch and resolve a conflict.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1073
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/47/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2161
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2046
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/47
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/47/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

8 - boost::system::error_code err;
9 - ba::write(*socket, ba::buffer(whole_message), err);
10 + try
11 + {
12 + boost::system::error_code err;
13 + ba::write(*socket, ba::buffer(whole_message), err);
14 + }
15 + catch (std::exception &)
16 + {
17 + // Don't care
18 + }

The try...catch block is redundant (this version of write doesn't throw - it populates err).

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Umm, so the bug is entirely fixed already?

That means we should reject the proposal.

Unmerged revisions

1073. By Daniel van Vugt on 2013-09-27

Merge latest dev branch and resolve a conflict.

1072. By Daniel van Vugt on 2013-09-20

Stop asio::write from throwing exceptions (SIGPIPE). It will only do so
after we've closed the socket, at which point we don't really care.
(LP: #1226139)

=== modified file 'src/server/frontend/socket_messenger.cpp'
--- src/server/frontend/socket_messenger.cpp	2013-09-25 14:18:18 +0000
+++ src/server/frontend/socket_messenger.cpp	2013-09-27 08:21:55 +0000
@@ -59,8 +59,15 @@
     // function has completed (if it would be executed asynchronously.
     // NOTE: we rely on this synchronous behavior as per the comment in
     // mf::SessionMediator::create_surface
-    boost::system::error_code err;
-    ba::write(*socket, ba::buffer(whole_message), err);
+    try
+    {
+        boost::system::error_code err;
+        ba::write(*socket, ba::buffer(whole_message), err);
+    }
+    catch (std::exception &)
+    {
+        // Don't care
+    }
 }
 
 void mfd::SocketMessenger::send_fds(std::vector<int32_t> const& fds)

=== modified file 'tests/unit-tests/frontend/CMakeLists.txt'
--- tests/unit-tests/frontend/CMakeLists.txt	2013-09-24 14:13:15 +0000
+++ tests/unit-tests/frontend/CMakeLists.txt	2013-09-27 08:21:55 +0000
@@ -8,6 +8,7 @@
   ${CMAKE_CURRENT_SOURCE_DIR}/test_resource_cache.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_session_mediator.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_socket_session.cpp
+  ${CMAKE_CURRENT_SOURCE_DIR}/test_socket_messenger.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_event_sender.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_global_event_sender.cpp
 )

=== added file 'tests/unit-tests/frontend/test_socket_messenger.cpp'
--- tests/unit-tests/frontend/test_socket_messenger.cpp	1970-01-01 00:00:00 +0000
+++ tests/unit-tests/frontend/test_socket_messenger.cpp	2013-09-27 08:21:55 +0000
@@ -0,0 +1,34 @@
+/*
+ * 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: Daniel van Vugt <[e-mail]>
+ */
+
+#include "src/server/frontend/socket_messenger.h"
+
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+
+using namespace mir::frontend::detail;
+using namespace boost::asio;
+
+TEST(SocketMessengerTest, write_failures_never_throw)
+{
+    io_service svc;
+    auto sock = std::make_shared<local::stream_protocol::socket>(svc);
+    SocketMessenger mess(sock);
+
+    EXPECT_NO_THROW( mess.send("foo") );
+}