Merge lp:~afrantzis/mir/fix-1340669-deadlock into lp:mir

[Alexandros Frantzis]

server: Fix deadlock when client with custom configuration disconnects

server: Fix deadlock when client with custom configuration disconnects

[Alexandros Frantzis (afrantzis)]

Oops pushed non-refactored version of BasicObservers tests, will push update (doesn't change test logic).

[Alan Griffiths (alan-griffiths)]

Agreed

review: Approve

[Alexandros Frantzis (afrantzis)]

> Oops pushed non-refactored version of BasicObservers tests, will push update
> (doesn't change test logic).

Pushed.

[Alan Griffiths (alan-griffiths)]

> > Oops pushed non-refactored version of BasicObservers tests, will push update
> > (doesn't change test logic).
>
> Pushed.

Better still

review: Approve

[Alan Griffiths (alan-griffiths)]

26 + while (current_item->next && (current_item = current_item->next));

could be

    while (current_item = current_item->next);

[Alexandros Frantzis (afrantzis)]

> 26 + while (current_item->next && (current_item = current_item->next));
>
> could be
>
> while (current_item = current_item->next);

Fixed (with extra parentheses to suppress warning/error).

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1767
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2137/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/920
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/926
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/919
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/658
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/658/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/656
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/656/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2966
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2966/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2060
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/9703

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

review: Approve (continuous-integration)

=== modified file 'include/shared/mir/basic_observers.h'
--- include/shared/mir/basic_observers.h	2014-07-10 13:46:41 +0000
+++ include/shared/mir/basic_observers.h	2014-07-11 15:00:30 +0000
@@ -104,8 +104,13 @@
 {
     ListItem* current_item = &head;
 
-    while (current_item)
+    do
     {
+        {
+            RecursiveReadLock lock{current_item->mutex};
+            if (current_item->observer != observer) continue;
+        }
+
         RecursiveWriteLock lock{current_item->mutex};
 
         if (current_item->observer == observer)
@@ -113,9 +118,8 @@
             current_item->observer.reset();
             return;
         }
-
-        current_item = current_item->next;
     }
+    while ((current_item = current_item->next));
 }
 }
 

=== modified file 'tests/acceptance-tests/CMakeLists.txt'
--- tests/acceptance-tests/CMakeLists.txt	2014-06-23 17:19:22 +0000
+++ tests/acceptance-tests/CMakeLists.txt	2014-07-11 15:00:30 +0000
@@ -38,6 +38,7 @@
   test_client_cursor_api.cpp
   test_large_messages.cpp
   test_client_surface_visibility.cpp
+  test_client_with_custom_display_config_deadlock.cpp
   ${GENERATED_PROTOBUF_SRCS}
   ${GENERATED_PROTOBUF_HDRS}
 )

=== added file 'tests/acceptance-tests/test_client_with_custom_display_config_deadlock.cpp'
--- tests/acceptance-tests/test_client_with_custom_display_config_deadlock.cpp	1970-01-01 00:00:00 +0000
+++ tests/acceptance-tests/test_client_with_custom_display_config_deadlock.cpp	2014-07-11 15:00:30 +0000
@@ -0,0 +1,72 @@
+/*
+ * Copyright © 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: Alexandros Frantzis <[e-mail]>
+ */
+
+#include "mir_toolkit/mir_client_library.h"
+
+#include "mir_test_framework/stubbed_server_configuration.h"
+#include "mir_test_framework/basic_client_server_fixture.h"
+
+#include <gtest/gtest.h>
+
+namespace mtf = mir_test_framework;
+
+namespace
+{
+
+struct StubServerConfig : mtf::StubbedServerConfiguration
+{
+    std::shared_ptr<mir::input::InputSender> the_input_sender()
+    {
+        return DefaultServerConfiguration::the_input_sender();
+    }
+};
+
+using ClientWithCustomDisplayConfiguration = mtf::BasicClientServerFixture<StubServerConfig>;
+
+}
+
+// Regression test for LP:#1340669
+// Test is not deterministic since we are testing a race, but failure can be
+// reproduced easily with repeated runs.
+TEST_F(ClientWithCustomDisplayConfiguration,
+       does_not_deadlock_server_with_existing_client_when_disconnecting)
+{
+    auto second_connection = mir_connect_sync(new_connection().c_str(), __PRETTY_FUNCTION__);
+    MirSurfaceParameters const request_params =
+    {
+        __PRETTY_FUNCTION__,
+        640, 480,
+        mir_pixel_format_abgr_8888,
+        mir_buffer_usage_hardware,
+        mir_display_output_id_invalid
+    };
+
+    auto second_surface = mir_connection_create_surface_sync(second_connection, &request_params);
+    ASSERT_TRUE(mir_surface_is_valid(second_surface));
+
+    auto configuration = mir_connection_create_display_config(connection);
+    mir_wait_for(mir_connection_apply_display_config(connection, configuration));
+    EXPECT_STREQ("", mir_connection_get_error_message(connection));
+    mir_display_config_destroy(configuration);
+
+    mir_connection_release(second_connection);
+
+    // Server (and therefore the test) will deadlock and won't be able to
+    // shut down without the fix. It's not ideal to deadlock on test failure,
+    // but it's the best check we have at the moment.
+}

=== modified file 'tests/unit-tests/CMakeLists.txt'
--- tests/unit-tests/CMakeLists.txt	2014-07-11 03:33:03 +0000
+++ tests/unit-tests/CMakeLists.txt	2014-07-11 15:00:30 +0000
@@ -19,6 +19,7 @@
   test_variable_length_array.cpp
   test_thread_name.cpp
   test_default_emergency_cleanup.cpp
+  test_basic_observers.cpp
 )
 
 add_subdirectory(options/)

=== added file 'tests/unit-tests/test_basic_observers.cpp'
--- tests/unit-tests/test_basic_observers.cpp	1970-01-01 00:00:00 +0000
+++ tests/unit-tests/test_basic_observers.cpp	2014-07-11 15:00:30 +0000
@@ -0,0 +1,124 @@
+/*
+ * Copyright © 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: Alexandros Frantzis <[e-mail]>
+ */
+
+#include "mir/basic_observers.h"
+#include "mir_test/wait_condition.h"
+
+#include <gtest/gtest.h>
+#include <gmock/gmock.h>
+
+namespace
+{
+
+struct DummyObserver
+{
+};
+
+struct DummyObservers : mir::BasicObservers<DummyObserver>
+{
+    using mir::BasicObservers<DummyObserver>::add;
+    using mir::BasicObservers<DummyObserver>::remove;
+    using mir::BasicObservers<DummyObserver>::for_each;
+};
+
+struct BasicObserversTest : testing::Test
+{
+    DummyObservers observers;
+
+    std::shared_ptr<DummyObserver> const observer1 = std::make_shared<DummyObserver>();
+    std::shared_ptr<DummyObserver> const observer2 = std::make_shared<DummyObserver>();
+};
+
+}
+
+TEST_F(BasicObserversTest, can_remove_observer_from_within_same_observer)
+{
+    using namespace testing;
+
+    observers.add(observer1);
+
+    observers.for_each(
+        [&] (std::shared_ptr<DummyObserver> const& observer)
+        {
+            observers.remove(observer);
+        });
+
+    int observers_seen = 0;
+
+    observers.for_each(
+        [&] (std::shared_ptr<DummyObserver> const&)
+        {
+            ++observers_seen;
+        });
+
+    EXPECT_THAT(observers_seen, Eq(0));
+}
+
+TEST_F(BasicObserversTest, can_remove_unused_observer_from_within_different_observer)
+{
+    using namespace testing;
+
+    observers.add(observer1);
+    observers.add(observer2);
+
+    int observers_seen = 0;
+
+    observers.for_each(
+        [&] (std::shared_ptr<DummyObserver> const&)
+        {
+            observers.remove(observer2);
+            ++observers_seen;
+        });
+
+    EXPECT_THAT(observers_seen, Eq(1));
+}
+
+TEST_F(BasicObserversTest,
+       can_remove_unused_observer_while_different_observer_is_used_in_different_thread)
+{
+    using namespace testing;
+
+    observers.add(observer1);
+    observers.add(observer2);
+
+    mir::test::WaitCondition first_observer_in_use;
+    mir::test::WaitCondition second_observer_removed;
+
+    int observers_seen = 0;
+
+    std::thread t{
+        [&]
+        {
+            observers.for_each(
+                [&] (std::shared_ptr<DummyObserver> const&)
+                {
+                    first_observer_in_use.wake_up_everyone();
+                    second_observer_removed.wait_for_at_most_seconds(3);
+                    EXPECT_TRUE(second_observer_removed.woken());
+                    ++observers_seen;
+                });
+        }};
+
+    first_observer_in_use.wait_for_at_most_seconds(3);
+    observers.remove(observer2);
+    second_observer_removed.wake_up_everyone();
+
+    t.join();
+
+    EXPECT_THAT(observers_seen, Eq(1));
+}