Merge lp:~raof/mir/no-hidden-rpc-in-bufferstream into lp:mir

[Chris Halse Rogers]

Eliminate the hidden RPC waits in MirBufferStream with NBS.

There were a variety of mir_buffer_stream_* calls which would implicitly block on RPC, particularly mir_buffer_stream_get_current_buffer() after
mir_connection_create_buffer_stream().

Resolve this by having the wait handles not signal until the buffer stream is in a valid state.

Another prerequisite for manual event dispatch.

This removes two unit tests from TestMirConnection - I did this because they require a lot of state set up now - surface creation doesn't complete until buffer stream creation is complete, which doesn't complete until the buffer vault receives a buffer in response to its alloc_buffer calls, and this buffer needs to be sufficiently genuine to make it through multiple layers of RPC/MirSurface dispatch.

Instead, there's a new acceptance test for this behaviour in https://code.launchpad.net/~raof/mir/acceptance-test-for-unfocus-on-surface-release/+merge/298098

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

FAILED: Continuous integration, rev:3555
https://mir-jenkins.ubuntu.com/job/mir-ci/1177/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/1325/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/1376
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/1367
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/1367
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/1339/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/1339
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/1339/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/1339
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1339/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/1339
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1339/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/1339
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/1339/artifact/output/*zip*/output.zip

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

review: Needs Fixing (continuous-integration)

[Kevin DuBois (kdub)]

(curiosity) why does manual dispatch need this?

lgtm otherwise. If there's future work in the area, I'm planning on condensing BufferVault and "NewBufferSemantics" into one class.

review: Approve

[Chris Halse Rogers (raof)]

Manual dispatch ran into this because you can't assume that RPC will occur at arbitrary times. Particularly - mir_buffer_stream_swap_buffers_sync() will first call mir_buffer_stream_swap_buffers() and then, in the same thread, pump the event loop until the wait handle resolves.

Except mir_buffer_stream_swap_buffers() calls get_current_buffer(), which blocks waiting for an IPC event that we'll never dispatch because we're blocked :).

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

PASSED: Continuous integration, rev:3556
https://mir-jenkins.ubuntu.com/job/mir-ci/1185/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/1344
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/1395
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/1386
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/1386
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/1358
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/1358/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/1358
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/1358/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/1358
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1358/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/1358
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1358/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/1358
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/1358/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Chris Halse Rogers (raof)]

Oh, hello! This doesn't actually ensure that the mir_surface_created callback is called only once the MirSurface is valid. Oops!

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

FAILED: Continuous integration, rev:3560
https://mir-jenkins.ubuntu.com/job/mir-ci/1230/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/1423/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/1474
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/1465
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/1465
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/1437/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/1437/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1437/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1437/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/1437/console

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

review: Needs Fixing (continuous-integration)

[Chris Halse Rogers (raof)]

...and it turns out that I really need to remove the hidden RPC *harder*...

Unmerged revisions

3560. By Chris Halse Rogers on 2016-07-05

Wait for default BufferStream creation before calling surface created callback.

Punt completion of the operation into the delayed-processing queue, where it can block on
the creation MirWaitHandle of the default BufferStream.

3559. By Chris Halse Rogers on 2016-07-05

Be more verbose when receiving an unexpected buffer.

This *should* never happen, so being verbose in the error message is low cost and potentially
high reward.

3558. By Chris Halse Rogers on 2016-07-05

MirProtobufRpcChannel: Don't destroy exception information on error.

3557. By Chris Halse Rogers on 2016-07-05

Send valid buffers in SurfaceLoop integration test.

This requires making StubBuffer a bit stubbier and actually packing the IPC messages.

3556. By Chris Halse Rogers on 2016-06-24

Fix NoTLSFuture so that the move-setter is actually accessible.

Because NoTLSPromise only had a set_value(T val) (wat‽) method, PromiseState's set_value(T&& val)
method was unreachable. Which is apparently why g++ didn't notice that it was broken.

3555. By Chris Halse Rogers on 2016-06-23

BufferStream: Do not signal creation wait handle until fully ready.

With NewBufferSemantics we were signalling the creation wait handle after the construction of the mcl::BufferStream,
but at this point the BufferVault does not necessarily have a buffer yet - that comes later as an event in response
to the allocate_buffer requests made during construction.

Instead, wait until the current buffer future<> has resolved before signalling that the BufferStream
is ready.

3554. By Chris Halse Rogers on 2016-06-23

Add missing gmock header.

testing::Eq and EXPECT_THAT are defined in Google Mock, not Google Test.

This only builds because MIR_USE_PRECOMPILED_HEADERS means that all our tests effectively
get the same set of headers included.

3553. By Chris Halse Rogers on 2016-06-23

Remove synthesised-focuse-event unit tests on MirConnection.

These require an ungodly amount of setup, so much so that they're basically
acceptance tests. The change to making sure buffer-streams are completely
constructed before we inform the client that they're completely constructed
results in these tests requiring *even more* setup.

Since we've just added real acceptance tests for this behaviour, just delete
these “unit” tests.

3552. By Chris Halse Rogers on 2016-06-20

Add NoTLSPromise<T>::or_else()

3551. By Chris Halse Rogers on 2016-06-20

NoTLSFuture is a class, damnit.

=== modified file 'src/client/buffer_factory.cpp'
--- src/client/buffer_factory.cpp	2016-06-06 11:51:31 +0000
+++ src/client/buffer_factory.cpp	2016-07-05 05:54:27 +0000
@@ -21,6 +21,7 @@
 #include <algorithm>
 #include <boost/throw_exception.hpp>
 #include "protobuf_to_native_buffer.h"
+#include <sstream>
 
 namespace mcl = mir::client;
 namespace geom = mir::geometry;
@@ -64,7 +65,16 @@
         });
 
     if (request_it == allocation_requests.end())
-        BOOST_THROW_EXCEPTION(std::logic_error("unrequested buffer received"));
+    {
+        std::stringstream message;
+        message << "Unrequested buffer received. Expected a buffer with one of the following sizes:" << std::endl;
+        for (auto const& request : allocation_requests)
+        {
+            message << "\t(" << request->size.width << "×" << request->size.height << ")" << std::endl;
+        }
+        message << "Received buffer has size: (" << buffer.width() << "×" << buffer.height() << ")" << std::endl;
+        BOOST_THROW_EXCEPTION(std::logic_error{message.str()});
+    }
 
     auto b = std::make_unique<Buffer>(
         (*request_it)->cb, (*request_it)->cb_context,

=== modified file 'src/client/buffer_stream.cpp'
--- src/client/buffer_stream.cpp	2016-06-06 11:51:31 +0000
+++ src/client/buffer_stream.cpp	2016-07-05 05:54:27 +0000
@@ -233,6 +233,7 @@
     MirWaitHandle next_buffer_wait_handle;
     bool server_connection_lost {false};
     MirWaitHandle scale_wait_handle;
+    MirWaitHandle construction_wait_handle;
     float scale_;
     geom::Size size_;
 };
@@ -300,6 +301,7 @@
 struct NewBufferSemantics : mcl::ServerBufferSemantics
 {
     NewBufferSemantics(
+        std::shared_ptr<MirWaitHandle> const& construction_wait_handle,
         std::shared_ptr<mcl::ClientBufferFactory> const& factory,
         std::shared_ptr<mcl::AsyncBufferFactory> const& mirbuffer_factory,
         std::shared_ptr<mcl::ServerBufferRequests> const& requests,
@@ -308,53 +310,82 @@
         unsigned int initial_nbuffers) :
         vault(factory, mirbuffer_factory, requests, surface_map, size, format, usage, initial_nbuffers),
         current(nullptr),
-        future(vault.withdraw()),
         size_(size)
     {
+        construction_wait_handle->expect_result();
+        future = vault.withdraw();
+        future.and_then(
+            [this, construction_wait_handle](std::shared_ptr<mcl::MirBuffer>&& new_buffer)
+            {
+                std::lock_guard<std::mutex> lk{current_buffer_mutex};
+
+                current = std::move(new_buffer);
+                construction_wait_handle->result_received();
+            });
+        future.or_else(
+            [this, construction_wait_handle](auto)
+            {
+                construction_wait_handle->result_received();
+            });
     }
 
     void deposit(mp::Buffer const&, mir::optional_value<geom::Size>, MirPixelFormat) override
     {
     }
 
-    void advance_current_buffer(std::unique_lock<std::mutex>& lk)
+    void ensure_current_is_valid_locked()
     {
-        lk.unlock();
-        auto c = future.get();
-        lk.lock();
-        current = c;
+        if (!current)
+        {
+            BOOST_THROW_EXCEPTION(
+                std::logic_error{
+                    "Attempt made to access current buffer before submit() has completed.\n"
+                    "After calling mir_buffer_stream_swap_buffers a client *must* wait for the operation to complete\n"
+                    "before accessing the current buffer. There is no current buffer until swap_buffers has completed."});
+        }
     }
 
     std::shared_ptr<mir::client::ClientBuffer> current_buffer() override
     {
-        std::unique_lock<std::mutex> lk(mutex);
-        if (!current)
-            advance_current_buffer(lk);
+        std::lock_guard<std::mutex> lk(current_buffer_mutex);
+        ensure_current_is_valid_locked();
         return current->client_buffer();
     }
 
     uint32_t current_buffer_id() override
     {
-        std::unique_lock<std::mutex> lk(mutex);
-        if (!current)
-            advance_current_buffer(lk);
+        std::lock_guard<std::mutex> lk(current_buffer_mutex);
+        ensure_current_is_valid_locked();
         return current->rpc_id();
     }
 
     MirWaitHandle* submit(std::function<void()> const& done, MirPixelFormat, int) override
     {
-        std::unique_lock<std::mutex> lk(mutex);
-        if (!current)
-            advance_current_buffer(lk);
-        auto c = current;
-        current = nullptr;
-        lk.unlock();
+        decltype(current) c;
+        {
+            std::lock_guard<std::mutex> lk(current_buffer_mutex);
+            ensure_current_is_valid_locked();
+            c = current;
+            current = nullptr;
+        }
 
         vault.deposit(c);
         auto wh = vault.wire_transfer_outbound(c, done);
         auto f = vault.withdraw();
-        lk.lock();
-        future = std::move(f);
+
+        {
+            std::lock_guard<std::mutex> lk{mutex};
+            future = std::move(f);
+
+            future.and_then(
+                [this](std::shared_ptr<mcl::MirBuffer>&& new_buffer)
+                {
+                    std::lock_guard<std::mutex> lk{current_buffer_mutex};
+
+                    current = std::move(new_buffer);
+                });
+        }
+
         return wh;
     }
 
@@ -403,10 +434,13 @@
         current_swap_interval = interval;
     }
 
+    // Future must be destroyed after vault to ensure any broken promise is handled
+    mir::client::NoTLSFuture<std::shared_ptr<mcl::MirBuffer>> future;
+
     mcl::BufferVault vault;
     std::mutex mutable mutex;
+    std::mutex current_buffer_mutex;
     std::shared_ptr<mcl::MirBuffer> current{nullptr};
-    mir::client::NoTLSFuture<std::shared_ptr<mcl::MirBuffer>> future;
     MirWaitHandle scale_wait_handle;
     int current_swap_interval = 1;
     geom::Size size_;
@@ -464,6 +498,7 @@
         else
         {
             buffer_depository = std::make_unique<NewBufferSemantics>(
+                creation_wait_handle,
                 client_platform->create_buffer_factory(), factory,
                 std::make_shared<Requests>(display_server, protobuf_bs->id().value()), map,
                 ideal_buffer_size, static_cast<MirPixelFormat>(protobuf_bs->pixel_format()), 
@@ -552,6 +587,7 @@
     else
     {
         buffer_depository = std::make_unique<NewBufferSemantics>(
+            creation_wait_handle,
             client_platform->create_buffer_factory(), factory,
             std::make_shared<Requests>(display_server, protobuf_bs->id().value()), map,
             ideal_buffer_size, static_cast<MirPixelFormat>(protobuf_bs->pixel_format()), 0, nbuffers);

=== modified file 'src/client/mir_connection.cpp'
--- src/client/mir_connection.cpp	2016-06-27 22:13:21 +0000
+++ src/client/mir_connection.cpp	2016-07-05 05:54:27 +0000
@@ -27,6 +27,8 @@
 #include "rpc/mir_basic_rpc_channel.h"
 #include "mir/dispatch/dispatchable.h"
 #include "mir/dispatch/threaded_dispatcher.h"
+#include "mir/dispatch/action_queue.h"
+#include "mir/dispatch/multiplexing_dispatchable.h"
 #include "mir/input/input_devices.h"
 #include "connection_configuration.h"
 #include "display_configuration.h"
@@ -287,9 +289,18 @@
         ping_handler{conf.the_ping_handler()},
         event_handler_register(conf.the_event_handler_register()),
         pong_callback(google::protobuf::NewPermanentCallback(&google::protobuf::DoNothing)),
-        eventloop{new md::ThreadedDispatcher{"RPC Thread", std::dynamic_pointer_cast<md::Dispatchable>(channel)}},
+        delayed_queue{std::make_unique<md::ActionQueue>()},
+        eventloop{std::make_unique<md::ThreadedDispatcher>(
+            "RPC Thread",
+             std::make_shared<md::MultiplexingDispatchable>(
+                std::initializer_list<std::shared_ptr<md::Dispatchable>>{
+                    std::dynamic_pointer_cast<md::Dispatchable>(channel),
+                    delayed_queue}))},
         nbuffers(get_nbuffers_from_env())
 {
+    // Need a second thread ensure RPC is not blocked if the delayed_queue blocks,
+    // as it does when waiting for surface creation.
+    eventloop->add_thread();
     connect_result->set_error("connect not called");
     {
         std::lock_guard<std::mutex> lock(connection_guard);
@@ -371,19 +382,19 @@
         return;
 
     auto surface_proto = request->response; 
-    auto callback = request->cb;
-    auto context = request->context;
     auto const& spec = request->spec;
     std::string name{spec.surface_name.is_set() ?
                      spec.surface_name.value() : ""};
 
     std::shared_ptr<mcl::ClientBufferStream> default_stream {nullptr};
+    std::shared_ptr<MirWaitHandle> buffer_stream_wh;
     if (surface_proto->buffer_stream().has_id())
     {
         try
         {
+            buffer_stream_wh = std::make_shared<MirWaitHandle>();
             default_stream = std::make_shared<mcl::BufferStream>(
-                this, request->wh, server, platform, surface_map, buffer_factory,
+                this, buffer_stream_wh, server, platform, surface_map, buffer_factory,
                 surface_proto->buffer_stream(), make_perf_report(logger), name,
                 mir::geometry::Size{surface_proto->width(), surface_proto->height()}, nbuffers);
             surface_map->insert(default_stream->rpc_id(), default_stream);
@@ -422,8 +433,13 @@
         surface_map->insert(mf::SurfaceId{surface_proto->id().value()}, surf);
     }
 
-    callback(surf.get(), context);
-    request->wh->result_received();
+    delayed_queue->enqueue(
+        [buffer_stream_wh, callback = request->cb, context = request->context, done = request->wh, surf, loop = eventloop.get()]()
+        {
+            mir_wait_for(buffer_stream_wh.get());
+            callback(surf.get(), context);
+            done->result_received();
+        });
 
     surface_requests.erase(request_it);
 }

=== modified file 'src/client/mir_connection.h'
--- src/client/mir_connection.h	2016-06-16 19:13:53 +0000
+++ src/client/mir_connection.h	2016-07-05 05:54:27 +0000
@@ -89,6 +89,7 @@
 namespace dispatch
 {
 class ThreadedDispatcher;
+class ActionQueue;
 }
 }
 
@@ -308,6 +309,8 @@
 
     std::unique_ptr<google::protobuf::Closure> const pong_callback;
 
+    std::shared_ptr<mir::dispatch::ActionQueue> const delayed_queue;
+
     std::unique_ptr<mir::dispatch::ThreadedDispatcher> const eventloop;
     
     mir::client::AtomicCallback<MirError const*> error_handler;

=== modified file 'src/client/no_tls_future-inl.h'
--- src/client/no_tls_future-inl.h	2016-05-03 06:55:25 +0000
+++ src/client/no_tls_future-inl.h	2016-07-05 05:54:27 +0000
@@ -35,6 +35,9 @@
 namespace client
 {
 template<typename T>
+class NoTLSFuture;
+
+template<typename T>
 class PromiseState
 {
 public:
@@ -51,15 +54,30 @@
     {
         std::lock_guard<std::mutex> lk(mutex);
         set = true;
-        value = val;
+        if (continuation)
+        {
+            value = val;
+            continuation(std::move(value));
+        }
+        else
+        {
+            value = val;
+        }
         cv.notify_all();
     }
 
-    void set_value(T && val)
+    void set_value(T&& val)
     {
         std::lock_guard<std::mutex> lk(mutex);
         set = true;
-        value = std::move(val);
+        if (continuation)
+        {
+            continuation(std::move(val));
+        }
+        else
+        {
+            value = val;
+        }
         cv.notify_all();
     }
 
@@ -79,7 +97,13 @@
     {
         std::lock_guard<std::mutex> lk(mutex);
         if (!set)
+        {
             broken = true;
+            if (exception_continuation)
+            {
+                exception_continuation(std::make_exception_ptr(std::runtime_error("broken_promise")));
+            }
+        }
         cv.notify_all();
     }
 
@@ -95,11 +119,35 @@
     bool set{false};
     bool broken{false};
     T value;
+    std::function<void(typename std::add_rvalue_reference<T>::type)> continuation;
+    std::function<void(std::exception_ptr const&)> exception_continuation;
+
+    friend class NoTLSFuture<T>;
+    void set_continuation(std::function<void(typename std::add_rvalue_reference<T>::type)> const& continuation)
+    {
+        std::lock_guard<std::mutex> lk{mutex};
+        if (set)
+        {
+            continuation(std::move(value));
+        }
+        this->continuation = continuation;
+    }
+
+    void set_exception_continuation(std::function<void(std::exception_ptr const&)> const& exception_continuation)
+    {
+        std::lock_guard<std::mutex> lk{mutex};
+        if (broken)
+        {
+            exception_continuation(std::make_exception_ptr(std::runtime_error("broken_promise")));
+        }
+        this->exception_continuation = exception_continuation;
+    }
 };
 
 template<typename T>
-struct NoTLSFuture
+class NoTLSFuture
 {
+public:
     NoTLSFuture() :
         state(nullptr) 
     {
@@ -138,6 +186,16 @@
         return value;
     }
 
+    void and_then(std::function<void(typename std::add_rvalue_reference<T>::type)> const& continuation)
+    {
+        state->set_continuation(continuation);
+    }
+
+    void or_else(std::function<void(std::exception_ptr const&)> const& handler)
+    {
+        state->set_exception_continuation(handler);
+    }
+
     template<class Rep, class Period>
     std::future_status wait_for(std::chrono::duration<Rep, Period> const& timeout_duration) const
     {
@@ -182,18 +240,30 @@
     NoTLSPromise(NoTLSPromise const&) = delete;
     NoTLSPromise operator=(NoTLSPromise const&) = delete;
 
-    void set_value(T value)
+    void set_value(T const& value)
     {
         state->set_value(value);
     }
 
+    void set_value(T&& value)
+    {
+        state->set_value(std::move(value));
+    }
+
     NoTLSFuture<T> get_future()
     {
+        if (future_retrieved)
+        {
+            //clang has problems with std::future_error::what() on vivid+overlay
+            BOOST_THROW_EXCEPTION(std::runtime_error{"future_already_retrieved"});
+        }
+        future_retrieved = true;
         return NoTLSFuture<T>(state);
     }
 
 private:
-    std::shared_ptr<PromiseState<T>> state; 
+    std::shared_ptr<PromiseState<T>> state;
+    bool future_retrieved{false};
 };
 }
 }

=== modified file 'src/client/rpc/mir_protobuf_rpc_channel.cpp'
--- src/client/rpc/mir_protobuf_rpc_channel.cpp	2016-06-02 05:33:50 +0000
+++ src/client/rpc/mir_protobuf_rpc_channel.cpp	2016-07-05 05:54:27 +0000
@@ -345,7 +345,7 @@
             {
                 for(auto i = 0; i < seq.buffer_request().buffer().fd_size(); i++)
                     close(seq.buffer_request().buffer().fd(i));
-                throw e;
+                throw;
             }
         }
         else

=== modified file 'tests/acceptance-tests/test_client_library.cpp'
--- tests/acceptance-tests/test_client_library.cpp	2016-06-08 20:40:53 +0000
+++ tests/acceptance-tests/test_client_library.cpp	2016-07-05 05:54:27 +0000
@@ -24,6 +24,7 @@
 #include "mir_test_framework/using_stub_client_platform.h"
 #include "mir_test_framework/any_surface.h"
 #include "mir/test/validity_matchers.h"
+#include "mir/test/signal.h"
 #include "src/include/common/mir/protobuf/protocol_version.h"
 
 #include "mir_protobuf.pb.h"
@@ -52,6 +53,7 @@
 namespace mf = mir::frontend;
 namespace mc = mir::compositor;
 namespace mtf = mir_test_framework;
+namespace mt = mir::test;
 namespace
 {
 struct ClientLibrary : mtf::HeadlessInProcessServer

=== modified file 'tests/acceptance-tests/test_client_surfaces.cpp'
--- tests/acceptance-tests/test_client_surfaces.cpp	2016-05-03 06:55:25 +0000
+++ tests/acceptance-tests/test_client_surfaces.cpp	2016-07-05 05:54:27 +0000
@@ -28,6 +28,7 @@
 #include "mir_test_framework/any_surface.h"
 #include "mir/test/validity_matchers.h"
 #include "mir/test/fake_shared.h"
+#include "mir/test/signal.h"
 
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
@@ -393,3 +394,39 @@
 
     mir_connection_release(im_connection);
 }
+
+TEST_F(ClientSurfaces, default_buffer_stream_is_fully_constructed_in_surface_created_callback)
+{
+    using namespace testing;
+    using namespace std::chrono_literals;
+
+    auto surface_spec = mir_connection_create_spec_for_normal_surface(
+        connection,
+        800, 600,
+        mir_pixel_format_argb_8888);
+
+    mt::Signal surface_released;
+    mir_surface_create(
+        surface_spec,
+        [](MirSurface* surf, void* ctx)
+        {
+            EXPECT_THAT(surf, IsValid());
+            auto buffer_stream = mir_surface_get_buffer_stream(surf);
+            MirGraphicsRegion region;
+            mir_buffer_stream_get_graphics_region(buffer_stream, &region);
+            EXPECT_THAT(region.vaddr, NotNull());
+
+            mir_surface_release(
+                surf,
+                [](MirSurface*, void* ctx)
+                {
+                    auto& done = *reinterpret_cast<mt::Signal*>(ctx);
+                    done.raise();
+                },
+                ctx);
+        },
+        &surface_released);
+    mir_surface_spec_release(surface_spec);
+
+    EXPECT_TRUE(surface_released.wait_for(10s));
+}

=== modified file 'tests/include/mir/test/doubles/stub_buffer.h'
--- tests/include/mir/test/doubles/stub_buffer.h	2016-01-29 08:18:22 +0000
+++ tests/include/mir/test/doubles/stub_buffer.h	2016-07-05 05:54:27 +0000
@@ -25,6 +25,7 @@
 #include "mir/graphics/buffer_id.h"
 #include <vector>
 #include <string.h>
+#include <atomic>
 
 namespace mir
 {
@@ -38,7 +39,7 @@
 public:
     StubBuffer()
         : StubBuffer{
-              create_native_buffer(),
+              create_native_buffer(geometry::Size{}),
               graphics::BufferProperties{
                   geometry::Size{},
                   mir_pixel_format_abgr_8888,
@@ -50,7 +51,7 @@
 
     StubBuffer(geometry::Size const& size)
         : StubBuffer{
-              create_native_buffer(),
+              create_native_buffer(size),
               graphics::BufferProperties{
                   size,
                   mir_pixel_format_abgr_8888,
@@ -78,12 +79,12 @@
     }
 
     StubBuffer(graphics::BufferProperties const& properties)
-        : StubBuffer{create_native_buffer(), properties, geometry::Stride{properties.size.width.as_int() * MIR_BYTES_PER_PIXEL(properties.format)}}
+        : StubBuffer{create_native_buffer(properties.size), properties, geometry::Stride{properties.size.width.as_int() * MIR_BYTES_PER_PIXEL(properties.format)}}
     {
     }
 
     StubBuffer(graphics::BufferID id)
-        : native_buffer(create_native_buffer()),
+        : native_buffer(create_native_buffer(geometry::Size{})),
           buf_size{},
           buf_pixel_format{mir_pixel_format_abgr_8888},
           buf_stride{},
@@ -98,7 +99,7 @@
           buf_size{properties.size},
           buf_pixel_format{properties.format},
           buf_stride{stride},
-          buf_id{graphics::BufferBasic::id()}
+          buf_id{++next_id}
     {
     }
 
@@ -138,8 +139,9 @@
     geometry::Stride const buf_stride;
     graphics::BufferID const buf_id;
     std::vector<unsigned char> written_pixels;
+    static std::atomic<unsigned int> next_id;
 
-    std::shared_ptr<graphics::NativeBuffer> create_native_buffer();
+    std::shared_ptr<graphics::NativeBuffer> create_native_buffer(geometry::Size const& size);
 };
 }
 }

=== modified file 'tests/integration-tests/test_surfaceloop.cpp'
--- tests/integration-tests/test_surfaceloop.cpp	2016-05-03 06:55:25 +0000
+++ tests/integration-tests/test_surfaceloop.cpp	2016-07-05 05:54:27 +0000
@@ -17,6 +17,7 @@
  */
 
 #include "mir_toolkit/mir_client_library.h"
+#include "mir/graphics/buffer_ipc_message.h"
 
 #include "mir/test/doubles/stub_buffer.h"
 #include "mir/test/doubles/stub_buffer_allocator.h"
@@ -49,30 +50,6 @@
 mg::BufferUsage const usage{mg::BufferUsage::hardware};
 mg::BufferProperties const buffer_properties{size, format, usage};
 
-
-class MockGraphicBufferAllocator : public mtd::StubBufferAllocator
-{
- public:
-    MockGraphicBufferAllocator()
-    {
-        using testing::_;
-        ON_CALL(*this, alloc_buffer(_))
-            .WillByDefault(testing::Invoke(this, &MockGraphicBufferAllocator::on_create_swapper));
-    }
-
-    MOCK_METHOD1(
-        alloc_buffer,
-        std::shared_ptr<mg::Buffer> (mg::BufferProperties const&));
-
-
-    std::shared_ptr<mg::Buffer> on_create_swapper(mg::BufferProperties const&)
-    {
-        return std::make_shared<mtd::StubBuffer>(::buffer_properties);
-    }
-
-    ~MockGraphicBufferAllocator() noexcept {}
-};
-
 class StubDisplay : public mtd::NullDisplay
 {
 public:
@@ -151,11 +128,13 @@
     {
     public:
 
-        CountingStubBuffer()
+        CountingStubBuffer(mg::BufferProperties const& properties)
+            : mtd::StubBuffer(properties)
         {
             std::lock_guard<std::mutex> lock{buffers_mutex};
             ++buffers_created;
             buffers_cv.notify_one();
+            std::cout << "Created buffer with size " << size() << std::endl;
         }
         ~CountingStubBuffer()
         {
@@ -173,9 +152,40 @@
     class StubGraphicBufferAllocator : public mtd::StubBufferAllocator
     {
      public:
-        std::shared_ptr<mg::Buffer> alloc_buffer(mg::BufferProperties const&) override
-        {
-            return std::make_shared<CountingStubBuffer>();
+        std::shared_ptr<mg::Buffer> alloc_buffer(mg::BufferProperties const& props) override
+        {
+            return std::make_shared<CountingStubBuffer>(props);
+        }
+    };
+
+    class StubIPCOperations : public mg::PlatformIpcOperations
+    {
+    public:
+        void pack_buffer(
+            mg::BufferIpcMessage& message,
+            mg::Buffer const& buffer,
+            mg::BufferIpcMsgType /*msg_type*/) const override
+        {
+            message.pack_size(buffer.size());
+        }
+
+        void
+        unpack_buffer(mg::BufferIpcMessage& /*message*/, mg::Buffer const& /*buffer*/) const override
+        {
+
+        }
+
+        std::shared_ptr<mir::graphics::PlatformIPCPackage> connection_ipc_package() override
+        {
+            return std::make_shared<mg::PlatformIPCPackage>();
+        }
+
+        mg::PlatformOperationMessage
+        platform_operation(
+            unsigned int const /*opcode*/,
+            mg::PlatformOperationMessage const& /*message*/) override
+        {
+            return mg::PlatformOperationMessage{};
         }
     };
 
@@ -193,6 +203,12 @@
         {
             return mir::make_module_ptr<StubDisplay>();
         }
+
+        mir::UniqueModulePtr<mg::PlatformIpcOperations> make_ipc_operations() const override
+        {
+            return mir::make_module_ptr<StubIPCOperations>();
+        }
+
     };
 
     std::shared_ptr<mg::Platform> the_graphics_platform()

=== modified file 'tests/mir_test_doubles/stub_buffer.cpp'
--- tests/mir_test_doubles/stub_buffer.cpp	2016-01-29 08:18:22 +0000
+++ tests/mir_test_doubles/stub_buffer.cpp	2016-07-05 05:54:27 +0000
@@ -26,12 +26,14 @@
 
 namespace mtd=mir::test::doubles;
 
-auto mtd::StubBuffer::create_native_buffer()
+auto mtd::StubBuffer::create_native_buffer(geometry::Size const& size)
 -> std::shared_ptr<graphics::NativeBuffer>
 {
 #if defined(MESA_KMS) || defined(MESA_X11)
-    return std::make_shared<StubGBMNativeBuffer>(geometry::Size{0,0});
+    return std::make_shared<StubGBMNativeBuffer>(size);
 #elif ANDROID
-    return std::make_shared<StubAndroidNativeBuffer>();
+    return std::make_shared<StubAndroidNativeBuffer>(size);
 #endif
 }
+
+std::atomic<unsigned int> mtd::StubBuffer::next_id{1};

=== modified file 'tests/unit-tests/client/CMakeLists.txt'
--- tests/unit-tests/client/CMakeLists.txt	2016-06-06 16:08:53 +0000
+++ tests/unit-tests/client/CMakeLists.txt	2016-07-05 05:54:27 +0000
@@ -23,6 +23,7 @@
   ${CMAKE_CURRENT_SOURCE_DIR}/test_mir_buffer.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_error_buffer.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_client_mir_error.cpp
+  ${CMAKE_CURRENT_SOURCE_DIR}/test_no_tls_future.cpp
 )
 
 if(MIR_TEST_PLATFORM STREQUAL "android")

=== modified file 'tests/unit-tests/client/test_client_buffer_stream.cpp'
--- tests/unit-tests/client/test_client_buffer_stream.cpp	2016-06-02 11:19:25 +0000
+++ tests/unit-tests/client/test_client_buffer_stream.cpp	2016-07-05 05:54:27 +0000
@@ -150,6 +150,7 @@
 struct ClientBufferStream : TestWithParam<bool>
 {
     ClientBufferStream()
+        : wait_handle{std::make_shared<MirWaitHandle>()}
     {
         ON_CALL(mock_factory, create_buffer(_,_,_))
             .WillByDefault(Return(std::make_shared<mtd::NullClientBuffer>()));

=== added file 'tests/unit-tests/client/test_no_tls_future.cpp'
--- tests/unit-tests/client/test_no_tls_future.cpp	1970-01-01 00:00:00 +0000
+++ tests/unit-tests/client/test_no_tls_future.cpp	2016-07-05 05:54:27 +0000
@@ -0,0 +1,111 @@
+/*
+ * Copyright © 2016 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: Christopher James Halse Rogers <[e-mail]>
+ */
+
+#include "src/client/no_tls_future-inl.h"
+
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+
+namespace mcl = mir::client;
+
+TEST(NoTLSFuture, and_then_calls_back_immediately_when_promise_is_already_fulfilled)
+{
+    mcl::NoTLSPromise<int> promise;
+    promise.set_value(1);
+
+    auto future = promise.get_future();
+    bool callback_called{false};
+
+    future.and_then([&callback_called](int) { callback_called = true;});
+    EXPECT_TRUE(callback_called);
+}
+
+TEST(NoTLSFuture, and_then_calls_back_with_correct_value_when_promise_is_already_fulfilled)
+{
+    constexpr int expected{0xfeed};
+
+    mcl::NoTLSPromise<int> promise;
+    promise.set_value(expected);
+
+    auto future = promise.get_future();
+    future.and_then(
+        [](int result)
+        {
+            using namespace testing;
+            EXPECT_THAT(result, Eq(expected));
+        });
+}
+
+TEST(NoTLSFuture, and_then_is_not_called_until_promise_is_fulfilled_by_copy)
+{
+    constexpr char const* expected{"And then nothing turned itself inside out"};
+
+    mcl::NoTLSPromise<std::string> promise;
+    auto future = promise.get_future();
+
+    bool called{false};
+    future.and_then(
+        [&called](std::string&& value)
+        {
+            using namespace testing;
+            called = true;
+            EXPECT_THAT(value, StrEq(expected));
+        });
+
+    EXPECT_FALSE(called);
+
+    std::string expected_copy{expected};
+
+    promise.set_value(expected_copy);
+    EXPECT_TRUE(called);
+}
+
+TEST(NoTLSFuture, and_then_is_not_called_until_promise_is_fulfilled_by_moving)
+{
+    constexpr char const* expected{"And then nothing turned itself inside out"};
+
+    mcl::NoTLSPromise<std::string> promise;
+    auto future = promise.get_future();
+
+    bool called{false};
+    future.and_then(
+        [&called](std::string&& value)
+        {
+            using namespace testing;
+            called = true;
+            EXPECT_THAT(value, StrEq(expected));
+        });
+
+    EXPECT_FALSE(called);
+
+    promise.set_value(std::move(std::string{expected}));
+    EXPECT_TRUE(called);
+}
+
+TEST(NoTLSFuture, or_else_is_called_when_promise_is_broken)
+{
+    auto promise = std::make_unique<mcl::NoTLSPromise<int>>();
+    auto future = promise->get_future();
+
+    bool called{false};
+    future.or_else([&called](auto) { called = true; });
+
+    EXPECT_FALSE(called);
+    promise.reset();
+    EXPECT_TRUE(called);
+}