Merge lp:~vanvugt/mir/reply-buffer-allocation-failures into lp:mir

[Daniel van Vugt]

If buffer allocation fails, remember to tell the client by setting
the error fields in the reply.

Also log a warning to the server log, since buffer allocation failure
means either the server is out of resources or the graphics driver
is not working yet (VirtualBox). Both of those are more problems with
the server than with the client.

For now, clients will still (since NBS was introduced anyway) ignore
such error reports and just hang forever waiting for their buffers (in
swap) that didn't get created. So more work is required still to make
clients recognise the error fields in buffer replies.

[Mir CI Bot (mir-ci-bot)]

PASSED: Continuous integration, rev:3823
https://mir-jenkins.ubuntu.com/job/mir-ci/2158/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/2796
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2861
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2853
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2853
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2853
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2825
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2825/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2825
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2825/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2825
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2825/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2825
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2825/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2825
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2825/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2825
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2825/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

It seems questionable to insert error handling here that (almost) duplicates that a couple of stack frames above (in invoke<>()). But I understand your desire to do "something".

review: Abstain

[Daniel van Vugt (vanvugt)]

If you mean more like this location, that might be doable:

https://code.launchpad.net/~vanvugt/mir/log-unhandled-exceptions/+merge/310398

But I'm running scared from that branch right now. Too much pointless argument over there.

[Daniel van Vugt (vanvugt)]

Actually I did have two good reasons for this branch based on the contention that previous branch caused:

(1) Buffer allocation failure is always a server error worth mentioning. Logging warnings about other exceptions might be overkill and inappropriate (?). Although in that previous branch I was gambling on that being untrue and we could just find out the truth eventually by reading our logs.

(2) Buffer allocation failures from the driver may contain very detailed and confusing error strings so should all be prefixed with something meaningful "Buffer allocation failed". Admittedly we could just do that by ensuring all graphics drivers provide nice strings.

Needs Information: I shall check if it's possible to make a generalised hybrid of that other branch and this one. It would be nice to reuse existing catches instead of adding new ones.

review: Needs Information

[Daniel van Vugt (vanvugt)]

Rejected. Replies with the error field set are already being sent from a different catch. Just I never realised because there is no logging in the client or server. The only definite problem is that libmirclient ignores error responses and waits for non-error responses forever.

Unmerged revisions

3823. By Daniel van Vugt on 2016-11-14

Enable the fix and the test now passes

3822. By Daniel van Vugt on 2016-11-14

Disable the fix and introduce a regression test

3821. By Daniel van Vugt on 2016-11-14

Cherry pick the bit that works from today's branch (just the server side)

=== modified file 'src/server/frontend/session_mediator.cpp'
--- src/server/frontend/session_mediator.cpp	2016-11-11 07:56:09 +0000
+++ src/server/frontend/session_mediator.cpp	2016-11-14 10:19:53 +0000
@@ -53,6 +53,8 @@
 #include "mir/fd.h"
 #include "mir/cookie/authority.h"
 #include "mir/module_properties.h"
+#include "mir/client_visible_error.h"
+#include "mir/log.h"
 
 #include "mir/geometry/rectangles.h"
 #include "protobuf_buffer_packer.h"
@@ -389,29 +391,53 @@
 
 void mf::SessionMediator::allocate_buffers( 
     mir::protobuf::BufferAllocation const* request,
-    mir::protobuf::Void*,
+    mir::protobuf::Void* result,
     google::protobuf::Closure* done)
 {
-    auto session = weak_session.lock();
-    if (!session)
-        BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
-
-    report->session_allocate_buffers_called(session->name());
-    for (auto i = 0; i < request->buffer_requests().size(); i++)
+    try
     {
-        auto const& req = request->buffer_requests(i);
-        mg::BufferProperties properties(
-            geom::Size{req.width(), req.height()},
-            static_cast<MirPixelFormat>(req.pixel_format()),
-           static_cast<mg::BufferUsage>(req.buffer_usage()));
+        auto session = weak_session.lock();
+        if (!session)
+            BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
 
-        auto id = session->create_buffer(properties);
-        if (request->has_id())
+        report->session_allocate_buffers_called(session->name());
+        for (auto i = 0; i < request->buffer_requests().size(); i++)
         {
-            auto stream = session->get_buffer_stream(mf::BufferStreamId(request->id().value()));
-            stream->associate_buffer(id);
+            auto const& req = request->buffer_requests(i);
+            mg::BufferProperties properties(
+                geom::Size{req.width(), req.height()},
+                static_cast<MirPixelFormat>(req.pixel_format()),
+               static_cast<mg::BufferUsage>(req.buffer_usage()));
+    
+            auto id = session->create_buffer(properties);
+            if (request->has_id())
+            {
+                auto stream = session->get_buffer_stream(mf::BufferStreamId(request->id().value()));
+                stream->associate_buffer(id);
+            }
         }
     }
+    // This is a misnomer: "ClientVisibleError" is not the only type of
+    // exception that clients should be notified of...
+    catch (mir::ClientVisibleError const& e)
+    {
+        auto const msg =
+            std::string{"Buffer allocation failed: "} + e.what();
+        result->set_error(msg);
+        auto se = result->mutable_structured_error();
+        se->set_domain(e.domain());
+        se->set_code(e.code());
+        mir::log_warning(msg);
+    }
+    // ... notify clients of traditional errors too. Which catch is used
+    // just depends on the type of exception the graphics driver likes to use.
+    catch (std::exception const& e)
+    {
+        auto const msg =
+            std::string{"Buffer allocation failed: "} + e.what();
+        result->set_error(msg);
+        mir::log_warning(msg);
+    }
     done->Run();
 }
  

=== modified file 'tests/unit-tests/frontend/test_session_mediator.cpp'
--- tests/unit-tests/frontend/test_session_mediator.cpp	2016-11-09 02:30:14 +0000
+++ tests/unit-tests/frontend/test_session_mediator.cpp	2016-11-14 10:19:53 +0000
@@ -200,8 +200,12 @@
     }
 
     
-    mg::BufferID create_buffer(mg::BufferProperties const&) override
+    mg::BufferID create_buffer(mg::BufferProperties const& prop) override
     {
+        if (!prop.size.width.as_int() || !prop.size.height.as_int())
+        {
+            throw std::runtime_error("Driver refused to allocate buffer");
+        }
         buffer_count++;
         return mg::BufferID{3};
     }
@@ -701,6 +705,34 @@
     EXPECT_THAT(stubbed_session->num_alloc_requests(), Eq(num_requests));
 }
 
+TEST_F(SessionMediator, buffer_allocation_failure_returns_error)
+{
+    mp::BufferAllocation request;
+    mp::BufferStreamId id;
+    id.set_value(0);
+    *request.mutable_id() = id;
+    mg::BufferProperties properties(geom::Size{0, 0},
+                                    mir_pixel_format_abgr_8888,
+                                    mg::BufferUsage::hardware);
+    for (int i = 0; i < 3; ++i)
+    {
+        auto buffer_request = request.add_buffer_requests();
+        buffer_request->set_width(properties.size.width.as_int());
+        buffer_request->set_height(properties.size.height.as_int());
+        buffer_request->set_pixel_format(properties.format);
+        buffer_request->set_buffer_usage((int)properties.usage);
+    }
+
+    mediator.connect(&connect_parameters, &connection, null_callback.get());
+    mediator.create_surface(&surface_parameters, &surface_response,
+                            null_callback.get());
+
+    mir::protobuf::Void result;
+    mediator.allocate_buffers(&request, &result, null_callback.get());
+    ASSERT_TRUE(result.has_error());
+    EXPECT_FALSE(result.error().empty());
+}
+
 TEST_F(SessionMediator, removes_buffer_from_the_session)
 {
     using namespace testing;