Merge lp:~raof/mir/fix-lock-inversion into lp:mir

[Chris Halse Rogers]

mcl::BufferVault: Fix lock inversion.

~BufferVault takes BufferVault::mutex and then calls AtomicCallback::set_callback() on each buffer,
which takes AtomicCallback::mutex.

The RPC subsystem calls buffer->callback(), which takes AtomicCallback::mutex and then calls
BufferVault::wire_transfer_inbound... which immediately takes BufferVault::mutex.

This is clearly a disaster waiting to happen.

Instead, move to a two-phase destruction approach:
1) Lock BufferVault::mutex, set a flag telling callbacks to bail, and collect all the existing
    buffers.
2) Drop the lock, and use buffer->set_callback() to wait on any thread already in a callback.
3) Don't bother telling the server to release any buffers; they're associated with the
   BufferStream that we're in the process of destroying, and will get cleaned up there.

This, unfortunately, moves the deadlock into a different place, but I think it's a correct change.

[Alan Griffiths (alan-griffiths)]

> The RPC subsystem calls buffer->callback(), which takes
> AtomicCallback::mutex and then calls BufferVault::wire_transfer_inbound...
> which immediately takes BufferVault::mutex.

This (pre-existing code) is hard to reason about, the extra logic doesn't help.

Would it be any more complex to make BufferVault::mutex a std::recursive_mutex? (The above is all on a single thread.)

review: Needs Information

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

PASSED: Continuous integration, rev:4214
https://mir-jenkins.ubuntu.com/job/mir-ci/3510/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/4803
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4986
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=artful/4975
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/4975
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4975
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/4840
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/4840/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4840
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4840/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/4840
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/4840/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/4840
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/4840/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4840
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4840/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/4840
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/4840/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4840
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4840/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/4840
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/4840/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Chris Halse Rogers (raof)]

It is on two different threads - the IPC thread calls buffer->callback (), the application thread calls ~BufferVault. I don't think a recursive mutex helps here?

On 26 July 2017 6:41:46 pm AEST, Alan Griffiths <email address hidden> wrote:
>Review: Needs Information
>
>> The RPC subsystem calls buffer->callback(), which takes
>> AtomicCallback::mutex and then calls
>BufferVault::wire_transfer_inbound...
>> which immediately takes BufferVault::mutex.
>
>This (pre-existing code) is hard to reason about, the extra logic
>doesn't help.
>
>Would it be any more complex to make BufferVault::mutex a
>std::recursive_mutex? (The above is all on a single thread.)
>--
>https://code.launchpad.net/~raof/mir/fix-lock-inversion/+merge/328069
>You are the owner of lp:~raof/mir/fix-lock-inversion.

[Alan Griffiths (alan-griffiths)]

> It is on two different threads - the IPC thread calls buffer->callback (), the
> application thread calls ~BufferVault. I don't think a recursive mutex helps
> here?

Ack. I misread what was happening.

review: Abstain

[Alan Griffiths (alan-griffiths)]

Nits, but non-blocking:

    bool disposing{false};

Perhaps not the clearest name. I don't have any great idea, but "shutting_down" seems marginally better.

~~~~

    try
    {
        ...
    }
    catch (...)
    {
    }

I guess this was there to ignore IPC errors in the destructor. If there are actually exceptions from the new code are being "eaten" something is badly wrong.

review: Approve

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

PASSED: Continuous integration, rev:4216
https://mir-jenkins.ubuntu.com/job/mir-ci/3511/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/4805
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4992
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=artful/4981
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/4981
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4981
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/4842
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/4842/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4842
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4842/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/4842
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/4842/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/4842
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/4842/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4842
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4842/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/4842
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/4842/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4842
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4842/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/4842
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/4842/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)

=== modified file 'src/client/buffer_vault.cpp'
--- src/client/buffer_vault.cpp	2017-07-17 02:44:34 +0000
+++ src/client/buffer_vault.cpp	2017-07-27 02:04:12 +0000
@@ -80,24 +80,63 @@
 mcl::BufferVault::~BufferVault()
 {
     buffer_factory->cancel_requests_with_context(this);
+
+    std::vector<std::shared_ptr<MirBuffer>> current_buffers;
+
     std::unique_lock<std::mutex> lk(mutex);
+
+    // Prevent callbacks from allocating new buffers
+    being_destroyed = true;
+
     for (auto& it : buffers)
-    try
+    if (auto map = surface_map.lock())
     {
-        if (auto map = surface_map.lock())
-        {
-            if (auto buffer = map->buffer(it.first))
-            {
-                buffer->set_callback(ignore_buffer, nullptr);
-            }
-        } 
-        if (!disconnected_)
-        {
-            free_buffer(it.first);
+        if (auto buffer = map->buffer(it.first))
+        {
+            /*
+             * Annoying wart:
+             *
+             * mir::AtomicCallback is atomic via locking, and we need it to be because
+             * we rely on the guarantee that after set_callback() returns no thread
+             * is in, or will enter, the previous callback.
+             *
+             * However!
+             *
+             * wire_transfer_inbound() is called from buffer->callback, and acquires
+             * BufferVault::mutex.
+             *
+             * This destuctor must call buffer->set_callback(), and has acquired
+             * BufferVault::mutex.
+             *
+             * So we've got a lock inversion.
+             *
+             * We can't unlock here, as wire_transfer_inbound() might mutate
+             * BufferVault::buffers, which we're iterating over.
+             *
+             * So store them up to process later...
+             */
+            current_buffers.push_back(buffer);
+
+            /*
+             * ...and erase them from the SurfaceMap.
+             *
+             * After this point, the RPC layer will ignore any messages about
+             * this buffer.
+             *
+             * We don't need to explicitly ask the server to free it; it'll be
+             * freed with the BufferStream
+             */
+            map->erase(it.first);
         }
     }
-    catch (...)
+    lk.unlock();
+
+    /*
+     * Now, ensure that no threads are in the existing buffer callbacks...
+     */
+    for (auto const& buffer : current_buffers)
     {
+        buffer->set_callback(ignore_buffer, nullptr);
     }
 }
 
@@ -246,6 +285,14 @@
 void mcl::BufferVault::wire_transfer_inbound(int buffer_id)
 {
     std::unique_lock<std::mutex> lk(mutex);
+
+    /*
+     * The BufferVault is in the process of being destroyed; there's no point in
+     * doing any processing and we should *certainly* not allocate any more buffers.
+     */
+    if (being_destroyed)
+        return;
+
     last_received_id = buffer_id;
     auto buffer = checked_buffer_from_map(buffer_id);
     auto inbound_size = buffer->size();

=== modified file 'src/client/buffer_vault.h'
--- src/client/buffer_vault.h	2017-05-08 03:04:26 +0000
+++ src/client/buffer_vault.h	2017-07-27 02:04:12 +0000
@@ -96,6 +96,7 @@
     int const usage;
 
     std::mutex mutex;
+    bool being_destroyed{false};
     BufferMap buffers;
     std::deque<NoTLSPromise<std::shared_ptr<MirBuffer>>> promises;
     geometry::Size size;

=== modified file 'tests/unit-tests/client/test_buffer_vault.cpp'
--- tests/unit-tests/client/test_buffer_vault.cpp	2017-05-17 04:48:46 +0000
+++ tests/unit-tests/client/test_buffer_vault.cpp	2017-07-27 02:04:12 +0000
@@ -156,16 +156,6 @@
     make_vault();
 }
 
-TEST_F(BufferVault, frees_the_buffers_we_actually_got)
-{
-    EXPECT_CALL(mock_requests, free_buffer(package.buffer_id()));
-    EXPECT_CALL(mock_requests, free_buffer(package2.buffer_id()));
-    auto vault = make_vault();
-
-    vault->wire_transfer_inbound(package.buffer_id());
-    vault->wire_transfer_inbound(package2.buffer_id());
-}
-
 TEST_F(BufferVault, withdrawing_and_never_filling_up_will_timeout)
 {
     using namespace std::literals::chrono_literals;
@@ -370,24 +360,6 @@
     }, std::exception);
 }
 
-TEST_F(BufferVault, makes_sure_rpc_calls_exceptions_are_caught_in_destructor)
-{
-    EXPECT_CALL(mock_requests, free_buffer(_))
-        .Times(1)
-        .WillOnce(Throw(std::runtime_error("")));
-    EXPECT_NO_THROW({
-        auto vault = make_vault();
-        vault->wire_transfer_inbound(package.buffer_id());
-    });
-}
-
-TEST_F(StartedBufferVault, skips_free_buffer_rpc_calls_if_disconnected)
-{
-    EXPECT_CALL(mock_requests, free_buffer(_))
-        .Times(0);
-    vault.disconnected();
-}
-
 TEST_F(StartedBufferVault, buffer_count_remains_the_same_after_scaling)
 {
     std::array<mp::Buffer, 3> buffers;