Change order of local endpoint clean-up

Summary
Change the order how a connection is cleaned-up.
Details
When a server -> client connection is broken the endpoint needs to be cleaned up,
before the corresponding client -> server connection is closed.
Otherwise it can happen that the later is restored and the former tried to be used to
acknowledge a subscription.

Author: kai-moritzkumkar

implementation/routing/src/routing_manager_client.cpp

@@ -2870,14 +2870,17 @@ void routing_manager_client::handle_client_error(client_t _client) {
             }
         }
 
+        // First ensure that the connection is dropped, before enforcing a
+        // reconnect from the client. Otherwise a client subscribe might
+        // be handled by a partially cleaned-up connection
+        remove_local(_client, true);
         // Remove the client from the local connections.
         {
             std::scoped_lock lock {receiver_mutex_};
             if (auto endpoint = std::dynamic_pointer_cast<server_endpoint>(receiver_)) {
                 endpoint->disconnect_from(_client);
             }
         }
-        remove_local(_client, true);
 
         // Request the host these services again.
         if ( state_ == inner_state_type_e::ST_REGISTERED ) {

test/network_tests/fake_socket_tests/helpers/base_fake_socket_fixture.cpp

@@ -105,4 +105,9 @@ void base_fake_socket_fixture::set_ignore_connections(std::string const& _app_na
     return socket_manager_->delay_message_processing(_from, _to, _delay);
 }
 
+[[nodiscard]] bool base_fake_socket_fixture::block_on_close_for(
+        std::string const& _from, std::optional<std::chrono::milliseconds> _from_block_time,
+        std::string const& _to, std::optional<std::chrono::milliseconds> _to_block_time) {
+    return socket_manager_->block_on_close_for(_from, _from_block_time, _to, _to_block_time);
+}
 }

test/network_tests/fake_socket_tests/helpers/base_fake_socket_fixture.hpp

@@ -145,6 +145,14 @@ struct base_fake_socket_fixture : ::testing::Test {
     [[nodiscard]] bool delay_message_processing(std::string const& _from, std::string const& _to,
                                                 bool _delay);
 
+    /**
+     * @see socket_manager::block_on_close_for()
+     **/
+    [[nodiscard]] bool block_on_close_for(std::string const& _from,
+                                          std::optional<std::chrono::milliseconds> _from_block_time,
+                                          std::string const& _to,
+                                          std::optional<std::chrono::milliseconds> _to_block_time);
+
 private:
     static std::shared_ptr<fake_socket_factory> factory_;
     std::shared_ptr<socket_manager> socket_manager_ {std::make_shared<socket_manager>()};

test/network_tests/fake_socket_tests/helpers/fake_tcp_socket_handle.cpp

@@ -9,6 +9,7 @@
 #include "test_logging.hpp"
 
 #include <span>
+#include <thread>
 #include <numeric>
 
 namespace vsomeip_v3::testing {
@@ -74,6 +75,16 @@ void fake_tcp_socket_handle::close() {
     if (remote) {
         remote->inner_close();
     }
+    auto block_time = [&] {
+        auto const lock = std::scoped_lock(mtx_);
+        return block_on_close_time_;
+    }();
+    if (block_time) {
+        TEST_LOG << "[fake-socket] delaying close procesesing for: " << socket_id_
+                 << " by: " << block_time->count() << "ms";
+        std::this_thread::sleep_for(*block_time);
+        TEST_LOG << "[fake-socket] continuing close procesesing for: " << socket_id_;
+    }
 }
 
 void fake_tcp_socket_handle::shutdown() {
@@ -126,6 +137,12 @@ void fake_tcp_socket_handle::delay_processing(bool _delay) {
     update_reception();
 }
 
+void fake_tcp_socket_handle::block_on_close_for(
+        std::optional<std::chrono::milliseconds> _block_time) {
+    auto const lock = std::scoped_lock(mtx_);
+    block_on_close_time_ = _block_time;
+}
+
 void fake_tcp_socket_handle::inner_close() {
     // called by the remote connected socket
     auto const lock = std::scoped_lock(mtx_);

test/network_tests/fake_socket_tests/helpers/fake_tcp_socket_handle.hpp

@@ -163,6 +163,12 @@ struct fake_tcp_socket_handle : std::enable_shared_from_this<fake_tcp_socket_han
      **/
     void delay_processing(bool _delay);
 
+    /**
+     * Lets the thread calling ::close sleep for _block_time after informing
+     * the connected socket about the closing.
+     **/
+    void block_on_close_for(std::optional<std::chrono::milliseconds> _block_time);
+
     void set_app_name(std::string const& _name);
     std::string get_app_name();
 
@@ -187,6 +193,7 @@ struct fake_tcp_socket_handle : std::enable_shared_from_this<fake_tcp_socket_han
     std::optional<Receptor> receptor_;
     boost::asio::ip::tcp::endpoint local_ep_;
     boost::asio::ip::tcp::endpoint remote_ep_;
+    std::optional<std::chrono::milliseconds> block_on_close_time_;
     std::mutex mtx_;
 };
 

test/network_tests/fake_socket_tests/helpers/socket_manager.cpp

@@ -349,4 +349,27 @@ void socket_manager::set_ignore_connections(std::string const& _app_name,
     }
     return false;
 }
+
+[[nodiscard]] bool socket_manager::block_on_close_for(
+        std::string const& _from, std::optional<std::chrono::milliseconds> _from_block_time,
+        std::string const& _to, std::optional<std::chrono::milliseconds> _to_block_time) {
+    auto const lock = std::scoped_lock(mtx_);
+    auto const cn = connection_name(_from, _to);
+    auto const it_connection = app_names_to_connection.find(cn);
+    if (it_connection == app_names_to_connection.end()) {
+        return false;
+    }
+    bool ret = true;
+    if (auto from = it_connection->second.first.lock(); from) {
+        from->block_on_close_for(_from_block_time);
+    } else {
+        ret = false;
+    }
+    if (auto to = it_connection->second.second.lock(); to) {
+        to->block_on_close_for(_to_block_time);
+    } else {
+        ret = false;
+    }
+    return ret;
+}
 }

test/network_tests/fake_socket_tests/helpers/socket_manager.hpp

@@ -114,6 +114,19 @@ class socket_manager : public std::enable_shared_from_this<socket_manager> {
      **/
     [[nodiscard]] bool delay_message_processing(std::string const& _from, std::string const& _to,
                                                 bool _delay);
+
+    /**
+     * searches for the _from -> _to connected sockets and demands from _from to block execution
+     * for _from_block_time when close is invoked, equivalent for _to with _to_block_time.
+     * @see fake_tcp_socket_handle::block_on_close_for() for further details.
+     *
+     * @return true, if all non nullopts could be forwarded
+     **/
+    [[nodiscard]] bool block_on_close_for(std::string const& _from,
+                                          std::optional<std::chrono::milliseconds> _from_block_time,
+                                          std::string const& _to,
+                                          std::optional<std::chrono::milliseconds> _to_block_time);
+
     /**
      * associates a fake_tcp_socket_handle to a io_context and therefore to an app_name.
      **/

test/network_tests/fake_socket_tests/test_connection_restoration.cpp

@@ -305,6 +305,30 @@ TEST_F(test_client_helper, client_ignores_server_connection_attempt_once) {
     EXPECT_TRUE(subscribe_to_event());
 }
 
+TEST_F(test_client_helper,
+       given_server_to_client_breaksdown_when_the_connection_is_re_established_then_the_subscription_confirmed) {
+    start_apps();
+    send_field_message();
+    ASSERT_TRUE(subscribe_to_field());
+    ASSERT_TRUE(client_->message_record_.wait_for(first_expected_field_message_));
+    client_->subscription_record_.clear();
+
+    // give the client a head start when cleaning the "sibling" connection, by holding back the
+    // server execution.
+    ASSERT_TRUE(block_on_close_for(client_name_, std::nullopt, server_name_,
+                                   std::chrono::milliseconds(200)));
+
+    // break the server->client connection. Notice that if the blocking above is in place,
+    // the "wrong" order of cleaning up server->client + client->server will lead to a race
+    // condition.
+    ASSERT_TRUE(
+            disconnect(server_name_, boost::asio::error::timed_out, client_name_, std::nullopt));
+
+    EXPECT_TRUE(client_->subscription_record_.wait_for(
+            event_subscription::successfully_subscribed_to(offered_field_),
+            std::chrono::seconds(6)));
+}
+
 struct test_single_connection_breakdown
     : test_client_helper,
       ::testing::WithParamInterface<std::pair<std::string, std::string>> { };