Merge lp:~kdub/mir/fix-1655929 into lp:mir

[Kevin DuBois]

start discarding calls in the MirProtobufRpcChannel immediately after getting a disconnect message. This ensures that no calls from other threads can send messages, (or more harmful, put messages in the PendingCallCache that won't get serviced by the server, or forcibly ended by the client shutdown code)

fixes: LP: #1655929

start discarding calls in the MirProtobufRpcChannel immediately after getting a disconnect message. This ensures that no calls from other threads can send messages, (or more harmful, put messages in the PendingCallCache that won't get serviced by the server, or forcibly ended by the client shutdown code)

fixes: LP: #1655929

Will probably want to see a few CI runs without this bug before TA.

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

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

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

+ std::mutex send_mutex;
std::atomic_bool discard{false};

Maybe I'm missing something, but we don't need a mutex to guard an atomic_bool, and I don't see any other use of it.

review: Needs Information

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

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

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

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

^^^
Bug 1646375, bug 1616312, bug 1646558

(1) Needs fixing: I agree the mutex should not be required with an atomic.

(2) Needs fixing: We've lost support for safe clean-up in the event that a disconnect is not what destroys the MirConnection object. Probably need to set discard=true inside wait_for_outstanding_calls() now, and rename it appropriately (an idea I toyed with in the past).

I loosely agree with removing discard_future_calls(), as I considered doing the same in the past. Can't remember why I decided in the end not to remove discard_future_calls...

So just some minor needs fixing. But also keep an eye on bug 1653658 to ensure this does not reintroduce it.

review: Needs Fixing

[Kevin DuBois (kdub)]

1) Sure, the remaining atomic wa just missed cleanup.
2) Added the function back. Combining the two have the challenge of coming up with a good name.

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

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

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

OK

review: Approve

[Alberto Aguirre (albaguirre)]

OK, I think this reduces the window for issues, but you can still have a

A main thread that does
1. async mir_surface_releae
2. mir_connect_release

Depending on timing:

The mir client dispatching thread while handling the surface release ack from the server,
can initiate more RPC calls (because window/surface release their resources) and puts some pending calls in the cache.

The client main thread can then call mir_connect_release, server responds, but pending calls above won't' be satisfied.

review: Approve

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

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

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

review: Approve (continuous-integration)

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

review: Approve (continuous-integration)

=== modified file 'src/client/rpc/mir_protobuf_rpc_channel.cpp'
--- src/client/rpc/mir_protobuf_rpc_channel.cpp	2017-01-24 12:33:31 +0000
+++ src/client/rpc/mir_protobuf_rpc_channel.cpp	2017-01-27 12:36:33 +0000
@@ -197,6 +197,7 @@
     google::protobuf::MessageLite* response,
     google::protobuf::Closure* complete)
 {
+    std::lock_guard<std::mutex> lk(discard_mutex);
     if (discard)
     {
        /*
@@ -211,6 +212,9 @@
        return;
     }
 
+    if (method_name == "disconnect")
+        discard = true;
+
     // Only send message when details saved for handling response
     std::vector<mir::Fd> fds;
     if (parameters->GetTypeName() == "mir.protobuf.BufferRequest")
@@ -244,6 +248,7 @@
 
 void mclr::MirProtobufRpcChannel::discard_future_calls()
 {
+    std::unique_lock<decltype(discard_mutex)> lk(discard_mutex);
     discard = true;
 }
 

=== modified file 'src/client/rpc/mir_protobuf_rpc_channel.h'
--- src/client/rpc/mir_protobuf_rpc_channel.h	2017-01-18 02:29:37 +0000
+++ src/client/rpc/mir_protobuf_rpc_channel.h	2017-01-27 12:36:33 +0000
@@ -104,7 +104,9 @@
 private:
     std::shared_ptr<RpcReport> const rpc_report;
     detail::PendingCallCache pending_calls;
-    std::atomic_bool discard{false};
+
+    std::mutex discard_mutex;
+    bool discard{false};
 
     static constexpr size_t size_of_header = 2;
     detail::SendBuffer header_bytes;