From 4f8359ff17308460f9c60314bc32178e5c96742b Mon Sep 17 00:00:00 2001 From: "Serhii Niukalov (GitHub)" <36993782+SNiukalov@users.noreply.github.com> Date: Sat, 24 Oct 2020 02:15:19 +0300 Subject: Fix sdl sends unexpected disconnect with resource constraint (#3516) * Rework processing of the OnExitApplication (RESOURCE_CONSTRAINT) notification * Update UTs according to changes Co-authored-by: sniukalov Co-authored-by: Aleksandr Kutsan Co-authored-by: jacobkeeler Co-authored-by: Andrii Kalinich --- .../hmi/on_exit_application_notification.cc | 1 - .../test/commands/hmi/hmi_notifications_test.cc | 47 ++++++++++++++++++-- .../src/application_manager_impl.cc | 6 +++ .../application_manager/src/rpc_service_impl.cc | 4 +- .../include/connection_handler/connection.h | 12 ++++++ .../connection_handler/connection_handler_impl.h | 7 +++ .../connection_handler/src/connection.cc | 12 +++++- .../src/connection_handler_impl.cc | 29 +++++++++++-- .../test/connection_handler_impl_test.cc | 39 +++++++++++++++++ .../connection_handler/connection_handler.h | 8 +++- .../include/protocol_handler/session_observer.h | 7 +++ .../test/protocol_handler/mock_session_observer.h | 1 + .../protocol_handler/src/protocol_handler_impl.cc | 1 + .../test/protocol_handler_tm_test.cc | 50 ++++++++++++++++++++++ 14 files changed, 211 insertions(+), 13 deletions(-) diff --git a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/on_exit_application_notification.cc b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/on_exit_application_notification.cc index 2b95d2e3e8..4a6a3df3b4 100644 --- a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/on_exit_application_notification.cc +++ b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/on_exit_application_notification.cc @@ -119,7 +119,6 @@ void OnExitApplicationNotification::Run() { MessageHelper::GetOnAppInterfaceUnregisteredNotificationToMobile( app_id, AppInterfaceUnregisteredReason::RESOURCE_CONSTRAINT); SendNotificationToMobile(message); - application_manager_.UnregisterApplication(app_id, Result::SUCCESS); return; } diff --git a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/test/commands/hmi/hmi_notifications_test.cc b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/test/commands/hmi/hmi_notifications_test.cc index 3efdee405a..400481f506 100644 --- a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/test/commands/hmi/hmi_notifications_test.cc +++ b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/test/commands/hmi/hmi_notifications_test.cc @@ -1028,14 +1028,12 @@ TEST_F(HMICommandsNotificationsTest, using ExitReason = hmi_apis::Common_ApplicationExitReason::eType; std::vector reason_list = { ExitReason::UNAUTHORIZED_TRANSPORT_REGISTRATION, - ExitReason::UNSUPPORTED_HMI_RESOURCE, - ExitReason::RESOURCE_CONSTRAINT}; + ExitReason::UNSUPPORTED_HMI_RESOURCE}; using UnregisteredReason = mobile_apis::AppInterfaceUnregisteredReason::eType; std::vector mobile_reason_list = { UnregisteredReason::APP_UNAUTHORIZED, - UnregisteredReason::UNSUPPORTED_HMI_RESOURCE, - UnregisteredReason::RESOURCE_CONSTRAINT}; + UnregisteredReason::UNSUPPORTED_HMI_RESOURCE}; std::vector::iterator it_mobile_reason = mobile_reason_list.begin(); @@ -1068,6 +1066,47 @@ TEST_F(HMICommandsNotificationsTest, } } +TEST_F(HMICommandsNotificationsTest, + OnExitApplicationNotificationResourceConstraintReason) { + auto message = CreateMessage(); + (*message)[am::strings::msg_params][am::strings::app_id] = kAppId_; + const auto notification = std::make_shared(); + (*notification)[am::strings::params][am::strings::function_id] = + static_cast( + mobile_apis::FunctionID::OnAppInterfaceUnregisteredID); + (*notification)[am::strings::params][am::strings::message_type] = + static_cast(am::MessageType::kNotification); + (*notification)[am::strings::params][am::strings::connection_key] = kAppId_; + + using ExitReason = hmi_apis::Common_ApplicationExitReason::eType; + auto hmi_reason = ExitReason::RESOURCE_CONSTRAINT; + + using UnregisteredReason = mobile_apis::AppInterfaceUnregisteredReason::eType; + auto mobile_reason = UnregisteredReason::RESOURCE_CONSTRAINT; + + (*message)[am::strings::msg_params][am::strings::reason] = hmi_reason; + const auto command = CreateCommand(message); + + (*notification)[am::strings::msg_params][am::strings::reason] = + static_cast(mobile_reason); + + am::plugin_manager::MockRPCPluginManager mock_rpc_plugin_manager_; + EXPECT_CALL(app_mngr_, GetPluginManager()) + .WillRepeatedly(ReturnRef(mock_rpc_plugin_manager_)); + + EXPECT_CALL(app_mngr_, application(kAppId_)).WillRepeatedly(Return(app_)); + EXPECT_CALL( + mock_message_helper_, + GetOnAppInterfaceUnregisteredNotificationToMobile(kAppId_, mobile_reason)) + .WillOnce(Return(notification)); + EXPECT_CALL(mock_rpc_service_, + ManageMobileCommand(notification, Command::SOURCE_SDL)); + EXPECT_CALL(app_mngr_, UnregisterApplication(_, _, _, _)).Times(0); + + ASSERT_TRUE(command->Init()); + command->Run(); +} + TEST_F(HMICommandsNotificationsTest, OnExitApplicationNotificationUnhandledReason) { MessageSharedPtr message = CreateMessage(); diff --git a/src/components/application_manager/src/application_manager_impl.cc b/src/components/application_manager/src/application_manager_impl.cc index 26557163d0..6b0ea39804 100644 --- a/src/components/application_manager/src/application_manager_impl.cc +++ b/src/components/application_manager/src/application_manager_impl.cc @@ -2181,6 +2181,12 @@ void ApplicationManagerImpl::OnServiceEndedCallback( is_unexpected_disconnect = false; break; } + case CloseSessionReason::kFinalMessage: { + reason = Result::SUCCESS; + is_resuming = false; + is_unexpected_disconnect = false; + break; + } default: { reason = Result::INVALID_ENUM; is_resuming = true; diff --git a/src/components/application_manager/src/rpc_service_impl.cc b/src/components/application_manager/src/rpc_service_impl.cc index 248db00729..34a0bed7af 100644 --- a/src/components/application_manager/src/rpc_service_impl.cc +++ b/src/components/application_manager/src/rpc_service_impl.cc @@ -467,8 +467,8 @@ void RPCServiceImpl::Handle(const impl::MessageToMobile message) { SDL_LOG_INFO("Message for mobile given away"); if (close_session) { - app_manager_.connection_handler().CloseSession(message->connection_key(), - connection_handler::kCommon); + app_manager_.connection_handler().CloseSession( + message->connection_key(), connection_handler::kFinalMessage); } } diff --git a/src/components/connection_handler/include/connection_handler/connection.h b/src/components/connection_handler/include/connection_handler/connection.h index 1a93509aca..8b286e1309 100644 --- a/src/components/connection_handler/include/connection_handler/connection.h +++ b/src/components/connection_handler/include/connection_handler/connection.h @@ -176,6 +176,17 @@ class Connection { */ uint32_t RemoveSession(uint8_t session_id); + /** + * @brief Called upon final message being sent for a session + */ + void OnFinalMessageCallback(); + + /** + * @brief Check whether final message was sent from this connection + * @return true if final message was sent by any session of this connection + */ + bool IsFinalMessageSent() const; + /** * @brief Adds uprotected service to session or * check protection to service has been started before @@ -378,6 +389,7 @@ class Connection { HeartBeatMonitor* heartbeat_monitor_; uint32_t heartbeat_timeout_; threads::Thread* heart_beat_monitor_thread_; + bool final_message_sent_; DISALLOW_COPY_AND_ASSIGN(Connection); }; diff --git a/src/components/connection_handler/include/connection_handler/connection_handler_impl.h b/src/components/connection_handler/include/connection_handler/connection_handler_impl.h index 08b64ec9b8..3bfa5b34b8 100644 --- a/src/components/connection_handler/include/connection_handler/connection_handler_impl.h +++ b/src/components/connection_handler/include/connection_handler/connection_handler_impl.h @@ -35,6 +35,7 @@ #include #include +#include #include #include @@ -272,6 +273,8 @@ class ConnectionHandlerImpl */ void OnMalformedMessageCallback(const uint32_t& connection_key) OVERRIDE; + void OnFinalMessageCallback(const uint32_t& connection_key) OVERRIDE; + /** * @brief Converts connection handle to transport type string used in * smartDeviceLink.ini file, e.g. "TCP_WIFI" @@ -646,6 +649,10 @@ class ConnectionHandlerImpl **/ void RemoveConnection(const ConnectionHandle connection_handle); + /** + * @brief Called when connection is closed. + * @param connection_id Connection unique identifier. + */ void OnConnectionEnded(const transport_manager::ConnectionUID connection_id); const uint8_t GetSessionIdFromSecondaryTransport( diff --git a/src/components/connection_handler/src/connection.cc b/src/components/connection_handler/src/connection.cc index 03915516c3..5bd72773c2 100644 --- a/src/components/connection_handler/src/connection.cc +++ b/src/components/connection_handler/src/connection.cc @@ -82,7 +82,8 @@ Connection::Connection(ConnectionHandle connection_handle, , connection_handle_(connection_handle) , connection_device_handle_(connection_device_handle) , primary_connection_handle_(0) - , heartbeat_timeout_(heartbeat_timeout) { + , heartbeat_timeout_(heartbeat_timeout) + , final_message_sent_(false) { SDL_LOG_AUTO_TRACE(); DCHECK(connection_handler_); @@ -162,6 +163,15 @@ uint32_t Connection::RemoveSession(uint8_t session_id) { return session_id; } +void Connection::OnFinalMessageCallback() { + SDL_LOG_AUTO_TRACE(); + final_message_sent_ = true; +} + +bool Connection::IsFinalMessageSent() const { + return final_message_sent_; +} + bool Connection::AddNewService(uint8_t session_id, protocol_handler::ServiceType service_type, const bool request_protection, diff --git a/src/components/connection_handler/src/connection_handler_impl.cc b/src/components/connection_handler/src/connection_handler_impl.cc index af32a741e1..549450dbe0 100644 --- a/src/components/connection_handler/src/connection_handler_impl.cc +++ b/src/components/connection_handler/src/connection_handler_impl.cc @@ -366,7 +366,7 @@ void ConnectionHandlerImpl::OnUnexpectedDisconnect( transport_manager::ConnectionUID connection_id, const transport_manager::CommunicationError& error) { SDL_LOG_AUTO_TRACE(); - + UNUSED(error); OnConnectionEnded(connection_id); } @@ -675,6 +675,25 @@ void ConnectionHandlerImpl::OnMalformedMessageCallback( CloseConnection(connection_handle); } +void ConnectionHandlerImpl::OnFinalMessageCallback( + const uint32_t& connection_key) { + SDL_LOG_AUTO_TRACE(); + + transport_manager::ConnectionUID connection_handle = 0; + uint8_t session_id = 0; + PairFromKey(connection_key, &connection_handle, &session_id); + + sync_primitives::AutoWriteLock connection_list_lock(connection_list_lock_); + ConnectionList::iterator connection_it = + connection_list_.find(connection_handle); + + if (connection_list_.end() != connection_it) { + SDL_LOG_DEBUG("OnFinalMessageCallback found connection " + << connection_handle); + connection_it->second->OnFinalMessageCallback(); + } +} + uint32_t ConnectionHandlerImpl::OnSessionEndedCallback( const transport_manager::ConnectionUID connection_handle, const uint8_t session_id, @@ -1716,6 +1735,10 @@ void ConnectionHandlerImpl::OnConnectionEnded( ending_connection_ = connection.get(); const SessionMap session_map = connection->session_map(); + const CloseSessionReason close_reason = + connection->IsFinalMessageSent() ? CloseSessionReason::kFinalMessage + : CloseSessionReason::kCommon; + for (SessionMap::const_iterator session_it = session_map.begin(); session_map.end() != session_it; ++session_it) { @@ -1731,9 +1754,7 @@ void ConnectionHandlerImpl::OnConnectionEnded( service_list.rbegin(); for (; service_list_itr != service_list.rend(); ++service_list_itr) { connection_handler_observer_->OnServiceEndedCallback( - session_key, - service_list_itr->service_type, - CloseSessionReason::kCommon); + session_key, service_list_itr->service_type, close_reason); } } ending_connection_ = NULL; diff --git a/src/components/connection_handler/test/connection_handler_impl_test.cc b/src/components/connection_handler/test/connection_handler_impl_test.cc index 3fbdf62474..9305eee2ee 100644 --- a/src/components/connection_handler/test/connection_handler_impl_test.cc +++ b/src/components/connection_handler/test/connection_handler_impl_test.cc @@ -855,6 +855,24 @@ TEST_F(ConnectionHandlerTest, OnConnectionClosed) { connection_handler_->OnConnectionClosed(uid_); } +TEST_F(ConnectionHandlerTest, OnFinalMessageCallback_OnConnectionClosed) { + AddTestDeviceConnection(); + AddTestSession(); + + connection_handler_test::MockConnectionHandlerObserver + mock_connection_handler_observer; + connection_handler_->set_connection_handler_observer( + &mock_connection_handler_observer); + + EXPECT_CALL(mock_connection_handler_observer, + OnServiceEndedCallback(connection_key_, kBulk, kFinalMessage)); + EXPECT_CALL(mock_connection_handler_observer, + OnServiceEndedCallback(connection_key_, kRpc, kFinalMessage)); + + connection_handler_->OnFinalMessageCallback(connection_key_); + connection_handler_->OnConnectionClosed(uid_); +} + TEST_F(ConnectionHandlerTest, OnUnexpectedDisconnect) { AddTestDeviceConnection(); AddTestSession(); @@ -874,6 +892,27 @@ TEST_F(ConnectionHandlerTest, OnUnexpectedDisconnect) { connection_handler_->OnUnexpectedDisconnect(uid_, err); } +TEST_F(ConnectionHandlerTest, OnFinalMessageCallback_OnUnexpectedDisconnect) { + AddTestDeviceConnection(); + AddTestSession(); + + connection_handler_test::MockConnectionHandlerObserver + mock_connection_handler_observer; + connection_handler_->set_connection_handler_observer( + &mock_connection_handler_observer); + + EXPECT_CALL(mock_connection_handler_observer, + OnServiceEndedCallback( + connection_key_, kBulk, CloseSessionReason::kFinalMessage)); + EXPECT_CALL(mock_connection_handler_observer, + OnServiceEndedCallback( + connection_key_, kRpc, CloseSessionReason::kFinalMessage)); + + connection_handler_->OnFinalMessageCallback(connection_key_); + transport_manager::CommunicationError err; + connection_handler_->OnUnexpectedDisconnect(uid_, err); +} + TEST_F(ConnectionHandlerTest, ConnectToDevice) { // Precondition const uint32_t dev_handle1 = 1; diff --git a/src/components/include/connection_handler/connection_handler.h b/src/components/include/connection_handler/connection_handler.h index c5f8c175e6..c5995fbbb8 100644 --- a/src/components/include/connection_handler/connection_handler.h +++ b/src/components/include/connection_handler/connection_handler.h @@ -48,7 +48,13 @@ */ namespace connection_handler { -enum CloseSessionReason { kCommon = 0, kFlood, kMalformed, kUnauthorizedApp }; +enum CloseSessionReason { + kCommon = 0, + kFlood, + kMalformed, + kUnauthorizedApp, + kFinalMessage +}; class ConnectionHandlerObserver; diff --git a/src/components/include/protocol_handler/session_observer.h b/src/components/include/protocol_handler/session_observer.h index cdf4267188..593ce8408c 100644 --- a/src/components/include/protocol_handler/session_observer.h +++ b/src/components/include/protocol_handler/session_observer.h @@ -178,6 +178,13 @@ class SessionObserver { */ virtual void OnMalformedMessageCallback(const uint32_t& connection_key) = 0; + /** + * @brief Callback function used by ProtocolHandler when the last message was + * sent for a mobile connection + * @param connection_key used by other components as an application identifier + */ + virtual void OnFinalMessageCallback(const uint32_t& connection_key) = 0; + /** * @brief Converts connection handle to transport type string used in * smartDeviceLink.ini file, e.g. "TCP_WIFI" diff --git a/src/components/include/test/protocol_handler/mock_session_observer.h b/src/components/include/test/protocol_handler/mock_session_observer.h index 3414153fc7..6bd67f6516 100644 --- a/src/components/include/test/protocol_handler/mock_session_observer.h +++ b/src/components/include/test/protocol_handler/mock_session_observer.h @@ -83,6 +83,7 @@ class MockSessionObserver : public ::protocol_handler::SessionObserver { void(const uint32_t& connection_key)); MOCK_METHOD1(OnMalformedMessageCallback, void(const uint32_t& connection_key)); + MOCK_METHOD1(OnFinalMessageCallback, void(const uint32_t& connection_key)); MOCK_CONST_METHOD1( TransportTypeProfileStringFromConnHandle, const std::string(transport_manager::ConnectionUID connection_handle)); diff --git a/src/components/protocol_handler/src/protocol_handler_impl.cc b/src/components/protocol_handler/src/protocol_handler_impl.cc index 7d83d6cd85..1db599e656 100644 --- a/src/components/protocol_handler/src/protocol_handler_impl.cc +++ b/src/components/protocol_handler/src/protocol_handler_impl.cc @@ -1069,6 +1069,7 @@ void ProtocolHandlerImpl::OnTMMessageSend(const RawMessagePtr message) { if (ready_to_close_connections_.end() != connection_it) { ready_to_close_connections_.erase(connection_it); + session_observer_.OnFinalMessageCallback(connection_handle); transport_manager_.Disconnect(connection_handle); return; } diff --git a/src/components/protocol_handler/test/protocol_handler_tm_test.cc b/src/components/protocol_handler/test/protocol_handler_tm_test.cc index 6edb77d0cd..41c16817b5 100644 --- a/src/components/protocol_handler/test/protocol_handler_tm_test.cc +++ b/src/components/protocol_handler/test/protocol_handler_tm_test.cc @@ -3550,6 +3550,56 @@ TEST_F(ProtocolHandlerImplTest, MalformedLimitVerification_NullCount) { EXPECT_TRUE(waiter->WaitFor(times, kAsyncExpectationsTimeout)); } +TEST_F(ProtocolHandlerImplTest, OnFinalMessageCallback) { + const auto protection = false; + const auto is_final_message = true; + const uint32_t total_data_size = 1; + UCharDataVector data(total_data_size); + + ProtocolFramePtr ptr(new protocol_handler::ProtocolPacket(connection_id, + PROTOCOL_VERSION_3, + protection, + FRAME_TYPE_SINGLE, + kControl, + FRAME_DATA_SINGLE, + session_id, + total_data_size, + message_id, + &data[0])); + + using RawFordMessageToMobile = protocol_handler::impl::RawFordMessageToMobile; + using Handler = protocol_handler::impl::ToMobileQueue::Handler; + + auto message = + std::make_shared(ptr, is_final_message); + auto raw_message = ptr->serializePacket(); + + auto handler = std::static_pointer_cast(protocol_handler_impl); + auto transport_manager_listener = + std::static_pointer_cast(protocol_handler_impl); + + EXPECT_CALL(session_observer_mock, + ProtocolVersionUsed(connection_id, session_id, An())) + .WillRepeatedly(Return(false)); + + EXPECT_CALL(session_observer_mock, + PairFromKey(raw_message->connection_key(), _, _)) + .WillRepeatedly( + DoAll(SetArgPointee<1>(connection_id), SetArgPointee<2>(session_id))); + + EXPECT_CALL(transport_manager_mock, SendMessageToDevice(_)) + .WillOnce(Return(E_SUCCESS)); + + EXPECT_CALL(session_observer_mock, OnFinalMessageCallback(connection_id)); + EXPECT_CALL(transport_manager_mock, Disconnect(connection_id)); + + handler->Handle(*message); + // Put the message to list of connections ready to close + transport_manager_listener->OnTMMessageSend(raw_message); + // Processing the list of connections ready to close + transport_manager_listener->OnTMMessageSend(raw_message); +} + TEST_F(ProtocolHandlerImplTest, SendEndServicePrivate_NoConnection_MessageNotSent) { // Expect check connection with ProtocolVersionUsed -- cgit v1.2.1