Merge lp:~mir-team/mir/enable-late-release into lp:mir

[Alan Griffiths]

client: don't allow shared libraries to be unloaded before platform-api calls connection_release()

client: don't allow shared libraries to be unloaded before platform-api calls connection_release()

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1775
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2182/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/981/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/987
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/976/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/703/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/701/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/3286/console

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1776
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2183/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/983
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/989
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/978
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/704
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/704/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/702
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/702/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/3290
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/3290/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2105
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/10008

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

(1) If the client library isn't guaranteeing this is cleaned up atexit for all clients already, I think it should. That's better than having to remember to reimplement the same in every client...

26 + // We should always release our connection
27 + atexit(&close_connection);

[Alan Griffiths (alan-griffiths)]

> (1) If the client library isn't guaranteeing this is cleaned up atexit for all
> clients already, I think it should. That's better than having to remember to

Without this MP we require clients to release connections before the end of main().

We have one client (platform-api) that releases a connection after main() exits.

This MP adds support for the way that client behaves.

All client side resources are released when the process exits. The server will detect the dead socket and releases resources corresponding in that process. What further cleanup are you suggesting is needed?

> reimplement the same in every client...
>
> 26 + // We should always release our connection
> 27 + atexit(&close_connection);

Yes, maybe I should put the example back the way it was - this is not the canonical way to implement a client.

[Alexandros Frantzis (afrantzis)]

The code looks ok.

I would prefer to revert basic_client.c to its previous form. If we want to be able to try this scenario we could add a flag to release after main (or perhaps a completely different example).

Ideally, though, we would have an automated test instead. Can the problem be reproduced deterministically? Couldn't we implement a test that launches a client process that releases after main and ensure that the process shuts down properly?

review: Needs Information

[Alan Griffiths (alan-griffiths)]

> The code looks ok.
>
> I would prefer to revert basic_client.c to its previous form. If we want to be
> able to try this scenario we could add a flag to release after main (or
> perhaps a completely different example).
>
> Ideally, though, we would have an automated test instead. Can the problem be
> reproduced deterministically? Couldn't we implement a test that launches a
> client process that releases after main and ensure that the process shuts down
> properly?

The problem is deterministic with the hacked example code and the original implementation.

I'm not sure how easy it is within the test framework as the failure occurs as the program is exiting. I.e. after the test suite exits, after function scoped statics are deleted, and among the namespace scoped destructor executions.

For now, I'll just revert the example.

[Alan Griffiths (alan-griffiths)]

I've added mir_test_client_release_at_exit - which will segfault if run with the development branch libmirclient and works with this branch.

[Robert Carr (robertcarr)]

LGTM

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1780
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2203/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1016
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1022
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1011
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/725
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/725/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/726
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/726/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/3443
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/3443/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2136
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/10160

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1781
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2205/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1023
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1029
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1016
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/727
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/727/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/728
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/728/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/3467
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/3467/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2141
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/10181

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

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

OK. It would be very nice to have an automated test, but not a blocker for this MP.

review: Approve

=== modified file 'examples/CMakeLists.txt'
--- examples/CMakeLists.txt	2014-06-11 16:11:32 +0000
+++ examples/CMakeLists.txt	2014-07-21 16:56:41 +0000
@@ -50,6 +50,9 @@
   ${CMAKE_THREAD_LIBS_INIT}
 )
 
+add_executable(mir_test_client_release_at_exit release_at_exit.c)
+target_link_libraries(mir_test_client_release_at_exit mirclient)
+
 add_executable(mir_demo_client_multiwin multiwin.c)
 target_link_libraries(mir_demo_client_multiwin mirclient)
 

=== added file 'examples/release_at_exit.c'
--- examples/release_at_exit.c	1970-01-01 00:00:00 +0000
+++ examples/release_at_exit.c	2014-07-21 16:56:41 +0000
@@ -0,0 +1,109 @@
+/*
+ * Copyright © 2012-2014 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: Alan Griffiths <[e-mail]>
+ */
+
+#include "mir_toolkit/mir_client_library.h"
+
+#include <assert.h>
+#include <string.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <getopt.h>
+
+static MirConnection *connection = 0;
+static MirSurface *surface = 0;
+
+static void connection_callback(MirConnection *new_connection, void *context)
+{
+    (void)context;
+    connection = new_connection;
+}
+
+static void surface_create_callback(MirSurface *new_surface, void *context)
+{
+    (void)context;
+    surface = new_surface;
+}
+
+static void surface_release_callback(MirSurface *old_surface, void *context)
+{
+    (void)old_surface;
+    (void)context;
+    surface = 0;
+}
+
+static void close_connection()
+{
+    if (connection)
+    {
+        mir_connection_release(connection);
+    }
+}
+
+
+void demo_client(const char* server)
+{
+    atexit(&close_connection);
+
+    mir_wait_for(mir_connect(server, __FILE__, connection_callback, NULL));
+    assert(mir_connection_is_valid(connection));
+
+    MirPixelFormat pixel_format;
+    unsigned int valid_formats;
+    mir_connection_get_available_surface_formats(connection, &pixel_format, 1, &valid_formats);
+    MirSurfaceParameters const request_params =
+        {NULL, 640, 480, pixel_format,
+         mir_buffer_usage_hardware, mir_display_output_id_invalid};
+
+    mir_wait_for(mir_connection_create_surface(connection, &request_params, surface_create_callback, NULL));
+    mir_wait_for(mir_surface_release(surface, surface_release_callback, NULL));
+}
+
+int main(int argc, char* argv[])
+{
+    // Some variables for holding command line options
+    char const *server = NULL;
+
+    // Parse the command line
+    {
+        int arg;
+        opterr = 0;
+        while ((arg = getopt (argc, argv, "hm:")) != -1)
+        {
+            switch (arg)
+            {
+            case 'm':
+                server = optarg;
+                break;
+
+            case '?':
+            case 'h':
+            default:
+                puts(argv[0]);
+                puts("Cutdown version of mir_demo_client_basic to show that");
+                puts("clients can release the connection after main() exits.");
+                puts("Usage:");
+                puts("    -m <Mir server socket>");
+                puts("    -h: this help text");
+                return -1;
+            }
+        }
+    }
+
+    demo_client(server);
+    return 0;
+}

=== modified file 'src/client/android/android_native_display_container.cpp'
--- src/client/android/android_native_display_container.cpp	2013-04-24 05:22:20 +0000
+++ src/client/android/android_native_display_container.cpp	2014-07-21 16:56:41 +0000
@@ -20,13 +20,35 @@
 
 #include "mir_toolkit/mir_client_library.h"
 
+#include <mutex>
+
 namespace mcl = mir::client;
 namespace mcla = mcl::android;
 
+namespace
+{
+// default_display_container needs to live until the library is unloaded
+std::mutex default_display_container_mutex;
+mcla::AndroidNativeDisplayContainer* default_display_container{nullptr};
+
+extern "C" int __attribute__((destructor)) destroy()
+{
+    std::lock_guard<std::mutex> lock(default_display_container_mutex);
+
+    delete default_display_container;
+
+    return 0;
+}
+}
+
 mcl::EGLNativeDisplayContainer& mcl::EGLNativeDisplayContainer::instance()
 {
-    static mcla::AndroidNativeDisplayContainer default_display_container;
-    return default_display_container;
+    std::lock_guard<std::mutex> lock(default_display_container_mutex);
+
+    if (!default_display_container)
+        default_display_container = new mcla::AndroidNativeDisplayContainer;
+
+    return *default_display_container;
 }
 
 mcla::AndroidNativeDisplayContainer::AndroidNativeDisplayContainer()

=== modified file 'src/client/connection_configuration.h'
--- src/client/connection_configuration.h	2014-05-29 13:21:08 +0000
+++ src/client/connection_configuration.h	2014-07-21 16:56:41 +0000
@@ -39,8 +39,11 @@
 class Logger;
 }
 
+class SharedLibrary;
+
 namespace client
 {
+std::shared_ptr<SharedLibrary>& libraries_cache(std::string const& lib_name);
 
 class ConnectionSurfaceMap;
 class Logger;
@@ -70,7 +73,6 @@
     ConnectionConfiguration(ConnectionConfiguration const&) = delete;
     ConnectionConfiguration& operator=(ConnectionConfiguration const&) = delete;
 };
-
 }
 }
 

=== modified file 'src/client/default_connection_configuration.cpp'
--- src/client/default_connection_configuration.cpp	2014-05-29 13:21:08 +0000
+++ src/client/default_connection_configuration.cpp	2014-07-21 16:56:41 +0000
@@ -45,10 +45,8 @@
 
 mir::SharedLibrary const* load_library(std::string const& libname)
 {
-    // There's no point in loading twice, and it isn't safe to unload...
-    static std::map<std::string, std::shared_ptr<mir::SharedLibrary>> libraries_cache;
 
-    if (auto& ptr = libraries_cache[libname])
+    if (auto& ptr = mcl::libraries_cache(libname))
     {
         return ptr.get();
     }

=== modified file 'src/client/mesa/mesa_native_display_container.cpp'
--- src/client/mesa/mesa_native_display_container.cpp	2014-03-06 06:05:17 +0000
+++ src/client/mesa/mesa_native_display_container.cpp	2014-07-21 16:56:41 +0000
@@ -46,12 +46,30 @@
 }
 
 }
-}
+
+// default_display_container needs to live until the library is unloaded
+std::mutex default_display_container_mutex;
+mclm::MesaNativeDisplayContainer* default_display_container{nullptr};
+
+extern "C" int __attribute__((destructor)) destroy()
+{
+    std::lock_guard<std::mutex> lock(default_display_container_mutex);
+
+    delete default_display_container;
+
+    return 0;
+}
+}
+
 
 mcl::EGLNativeDisplayContainer& mcl::EGLNativeDisplayContainer::instance()
 {
-    static mclm::MesaNativeDisplayContainer default_display_container;
-    return default_display_container;
+    std::lock_guard<std::mutex> lock(default_display_container_mutex);
+
+    if (!default_display_container)
+        default_display_container = new mclm::MesaNativeDisplayContainer;
+
+    return *default_display_container;
 }
 
 mclm::MesaNativeDisplayContainer::MesaNativeDisplayContainer()

=== modified file 'src/client/mir_connection.cpp'
--- src/client/mir_connection.cpp	2014-06-16 14:47:14 +0000
+++ src/client/mir_connection.cpp	2014-07-21 16:56:41 +0000
@@ -63,11 +63,48 @@
     MirDisplayConfiguration* const config;
 };
 
+using LibrariesCache = std::map<std::string, std::shared_ptr<mir::SharedLibrary>>;
+
 std::mutex connection_guard;
-std::unordered_set<MirConnection*> valid_connections;
+MirConnection* valid_connections{nullptr};
+// There's no point in loading twice, and it isn't safe to
+// unload while there are valid connections
+std::map<std::string, std::shared_ptr<mir::SharedLibrary>>* libraries_cache_ptr{nullptr};
+}
+
+std::shared_ptr<mir::SharedLibrary>& mcl::libraries_cache(std::string const& libname)
+{
+    std::lock_guard<std::mutex> lock(connection_guard);
+
+    if (!libraries_cache_ptr)
+        libraries_cache_ptr = new LibrariesCache;
+
+    return (*libraries_cache_ptr)[libname];
+}
+
+MirConnection::Deregisterer::~Deregisterer()
+{
+    std::lock_guard<std::mutex> lock(connection_guard);
+
+    for (auto current = &valid_connections; *current; current = &(*current)->next_valid)
+    {
+        if (self == *current)
+        {
+            *current = self->next_valid;
+            break;
+        }
+    }
+
+    // When the last valid connection goes we can clear the libraries cache
+    if (!valid_connections)
+    {
+        delete libraries_cache_ptr;
+        libraries_cache_ptr = nullptr;
+    }
 }
 
 MirConnection::MirConnection(std::string const& error_message) :
+    deregisterer{this},
     channel(),
     server(0),
     error_message(error_message)
@@ -76,6 +113,7 @@
 
 MirConnection::MirConnection(
     mir::client::ConnectionConfiguration& conf) :
+        deregisterer{this},
         channel(conf.the_rpc_channel()),
         server(channel.get(), ::google::protobuf::Service::STUB_DOESNT_OWN_CHANNEL),
         logger(conf.the_logger()),
@@ -89,7 +127,8 @@
     connect_result.set_error("connect not called");
     {
         std::lock_guard<std::mutex> lock(connection_guard);
-        valid_connections.insert(this);
+        next_valid = valid_connections;
+        valid_connections = this;
     }
 }
 
@@ -99,11 +138,6 @@
     // But, if after 500ms we don't get a call, assume it won't happen.
     connect_wait_handle.wait_for_pending(std::chrono::milliseconds(500));
 
-    {
-        std::lock_guard<std::mutex> lock(connection_guard);
-        valid_connections.erase(this);
-    }
-
     std::lock_guard<decltype(mutex)> lock(mutex);
     if (connect_result.has_platform())
     {
@@ -317,13 +351,18 @@
 bool MirConnection::is_valid(MirConnection *connection)
 {
     {
-        std::lock_guard<std::mutex> lock(connection_guard);
-        if (valid_connections.count(connection) == 0)
-           return false;
+        std::unique_lock<std::mutex> lock(connection_guard);
+        for (auto current = valid_connections; current; current = current->next_valid)
+        {
+            if (connection == current)
+            {
+                lock.unlock();
+                std::lock_guard<decltype(connection->mutex)> lock(connection->mutex);
+                return !connection->connect_result.has_error();
+            }
+        }
     }
-
-    std::lock_guard<decltype(connection->mutex)> lock(connection->mutex);
-    return !connection->connect_result.has_error();
+    return false;
 }
 
 void MirConnection::populate(MirPlatformPackage& platform_package)

=== modified file 'src/client/mir_connection.h'
--- src/client/mir_connection.h	2014-06-11 16:11:32 +0000
+++ src/client/mir_connection.h	2014-07-21 16:56:41 +0000
@@ -134,6 +134,10 @@
     mir::protobuf::DisplayServer& display_server();
 
 private:
+    // MUST be first data member so it is destroyed last.
+    struct Deregisterer
+    { MirConnection* const self; ~Deregisterer(); } deregisterer;
+
     std::mutex mutex; // Protects all members of *this (except release_wait_handles)
 
     std::shared_ptr<google::protobuf::RpcChannel> const channel;
@@ -174,6 +178,8 @@
 
     struct SurfaceRelease;
 
+    MirConnection* next_valid{nullptr};
+
     void set_error_message(std::string const& error);
     void done_disconnect();
     void connected(mir_connected_callback callback, void * context);