From ec8b48590b89c2ba1dced1e33ad8565970829a5a Mon Sep 17 00:00:00 2001 From: "Andrey Oleynik (GitHub)" Date: Wed, 22 Nov 2017 15:49:42 +0200 Subject: Fixes RPCs holding for applications being switched --- .../include/application_manager/command_holder.h | 23 ++++-- .../application_manager/command_holder_impl.h | 41 +++++++---- .../src/application_manager_impl.cc | 40 ++++++++--- .../application_manager/src/command_holder_impl.cc | 84 +++++++++++++++++++--- .../test/application_manager_impl_mock_hmi_test.cc | 4 ++ .../test/command_holder_test.cc | 84 +++++++++++++++++----- 6 files changed, 221 insertions(+), 55 deletions(-) diff --git a/src/components/application_manager/include/application_manager/command_holder.h b/src/components/application_manager/include/application_manager/command_holder.h index 02ae91e30c..4023f184b0 100644 --- a/src/components/application_manager/include/application_manager/command_holder.h +++ b/src/components/application_manager/include/application_manager/command_holder.h @@ -33,16 +33,22 @@ #pragma once #include +#include "application_manager/application.h" #include "smart_objects/smart_object.h" #include "utils/shared_ptr.h" namespace application_manager { /** * @brief The CommandHolder class should hold commands for particular - * application specified by its id + * application until certain event happens */ class CommandHolder { public: + /** + * @brief The CommandType enum defines type of command to suspend or resume + */ + enum class CommandType { kHmiCommand, kMobileCommand }; + /** * @brief ~CommandsHolder destructor */ @@ -51,23 +57,26 @@ class CommandHolder { /** * @brief Suspend collects command for specific application policy id * internally - * @param policy_app_id Application policy id + * @param application Application pointer + * @param type Command type * @param command Command */ - virtual void Suspend(const std::string& policy_app_id, + virtual void Suspend(ApplicationSharedPtr application, + CommandType type, smart_objects::SmartObjectSPtr command) = 0; /** * @brief Resume send all collected commands for further processing and * removes them afterward - * @param policy_app_id Application policy id + * @param application Application pointer + * @param type Command type */ - virtual void Resume(const std::string& policy_app_id) = 0; + virtual void Resume(ApplicationSharedPtr application, CommandType type) = 0; /** * @brief Clear removes all collected commands w/o processing - * @param policy_app_id Application policy id + * @param application Application pointer */ - virtual void Clear(const std::string& policy_app_id) = 0; + virtual void Clear(ApplicationSharedPtr application) = 0; }; } // application_manager diff --git a/src/components/application_manager/include/application_manager/command_holder_impl.h b/src/components/application_manager/include/application_manager/command_holder_impl.h index a9dfa46bc7..a519f310c3 100644 --- a/src/components/application_manager/include/application_manager/command_holder_impl.h +++ b/src/components/application_manager/include/application_manager/command_holder_impl.h @@ -36,6 +36,8 @@ #include #include +#include +#include "application_manager/application.h" #include "smart_objects/smart_object.h" #include "utils/lock.h" #include "utils/shared_ptr.h" @@ -44,10 +46,9 @@ namespace application_manager { class ApplicationManager; /** - * @brief The CommandHolderImpl class should hold HMI commands for particular - * application specified by its policy id during application transport switching - * process and sends for processing after switching is completed successfully - * or drop otherwise + * @brief The CommandHolderImpl class should hold commands for particular + * application during application transport switching process and sends for + * processing after switching is completed successfully or drops otherwise */ class CommandHolderImpl : public CommandHolder { public: @@ -59,32 +60,48 @@ class CommandHolderImpl : public CommandHolder { /** * @brief Suspend collects command for specific application id internally - * @param policy_app_id Policy id of application + * @param application Application pointer + * @param type Command type * @param command Command */ - void Suspend(const std::string& policy_app_id, + void Suspend(ApplicationSharedPtr application, + CommandType type, smart_objects::SmartObjectSPtr command) FINAL; /** * @brief Resume sends all collected HMI commands to ApplicationManager * for further processing - * @param policy_app_id Policy id of application + * @param application Application pointer + * @param type Command type */ - void Resume(const std::string& policy_app_id) FINAL; + void Resume(ApplicationSharedPtr application, CommandType type) FINAL; /** * @brief Clear removes all commands collected for specific application id - * @param policy_app_id Policy application id + * @param application Application pointer */ - void Clear(const std::string& policy_app_id) FINAL; + void Clear(ApplicationSharedPtr application) FINAL; private: + /** + * @brief ResumeHmiCommand sends suspended HMI commands for processing + * @param application Application which commands to process + */ + void ResumeHmiCommand(ApplicationSharedPtr app); + + /** + * @brief ResumeMobileCommand sends suspended mobile commands for processing + * @param application Application which commands to process + */ + void ResumeMobileCommand(ApplicationSharedPtr application); + using AppCommands = - std::map > >; ApplicationManager& app_manager_; sync_primitives::Lock commands_lock_; - AppCommands app_commands_; + AppCommands app_mobile_commands_; + AppCommands app_hmi_commands_; }; } // application_manager diff --git a/src/components/application_manager/src/application_manager_impl.cc b/src/components/application_manager/src/application_manager_impl.cc index 03e9a7d95e..f9fa396d80 100644 --- a/src/components/application_manager/src/application_manager_impl.cc +++ b/src/components/application_manager/src/application_manager_impl.cc @@ -391,7 +391,9 @@ void ApplicationManagerImpl::OnApplicationRegistered(ApplicationSharedPtr app) { } void ApplicationManagerImpl::OnApplicationSwitched(ApplicationSharedPtr app) { - commands_holder_->Resume(app->policy_app_id()); + LOG4CXX_AUTO_TRACE(logger_); + commands_holder_->Resume(app, CommandHolder::CommandType::kMobileCommand); + commands_holder_->Resume(app, CommandHolder::CommandType::kHmiCommand); } bool ApplicationManagerImpl::IsAppTypeExistsInFullOrLimited( @@ -1102,8 +1104,7 @@ void ApplicationManagerImpl::OnDeviceSwitchingFinish( for (auto app_it = reregister_wait_list_.begin(); app_it != reregister_wait_list_.end(); ++app_it) { - auto app = *app_it; - commands_holder_->Clear(app->policy_app_id()); + auto app = *app_it; UnregisterApplication(app->app_id(), mobile_apis::Result::INVALID_ENUM, is_resuming, @@ -1823,6 +1824,17 @@ bool ApplicationManagerImpl::ManageMobileCommand( return false; } + const uint32_t connection_key = static_cast( + (*message)[strings::params][strings::connection_key].asUInt()); + + auto app_ptr = application(connection_key); + if (app_ptr && IsAppInReconnectMode(app_ptr->policy_app_id())) { + commands_holder_->Suspend(app_ptr, + CommandHolder::CommandType::kMobileCommand, + message); + return true; + } + mobile_apis::FunctionID::eType function_id = static_cast( (*message)[strings::params][strings::function_id].asInt()); @@ -1833,9 +1845,6 @@ bool ApplicationManagerImpl::ManageMobileCommand( ? (*message)[strings::params][strings::correlation_id].asUInt() : 0; - uint32_t connection_key = - (*message)[strings::params][strings::connection_key].asUInt(); - int32_t protocol_type = (*message)[strings::params][strings::protocol_type].asUInt(); @@ -2052,10 +2061,18 @@ bool ApplicationManagerImpl::ManageHMICommand( return false; } - auto app = application(command->connection_key()); - if (app && IsAppInReconnectMode(app->policy_app_id())) { - commands_holder_->Suspend(app->policy_app_id(), message); - return true; + if ((*message).keyExists(strings::msg_params) && + (*message)[strings::msg_params].keyExists(strings::app_id)) { + const auto connection_key = + (*message)[strings::msg_params][strings::app_id].asUInt(); + + auto app = application(static_cast(connection_key)); + if (app && IsAppInReconnectMode(app->policy_app_id())) { + commands_holder_->Suspend(app, + CommandHolder::CommandType::kHmiCommand, + message); + return true; + } } int32_t message_type = @@ -3123,6 +3140,9 @@ void ApplicationManagerImpl::UnregisterApplication( SendUpdateAppList(); } } + + commands_holder_->Clear(app_to_remove); + if (audio_pass_thru_active_) { // May be better to put this code in MessageHelper? EndAudioPassThrough(); diff --git a/src/components/application_manager/src/command_holder_impl.cc b/src/components/application_manager/src/command_holder_impl.cc index dfb100c802..d5700f512e 100644 --- a/src/components/application_manager/src/command_holder_impl.cc +++ b/src/components/application_manager/src/command_holder_impl.cc @@ -32,38 +32,102 @@ #include "application_manager/command_holder_impl.h" #include "application_manager/application_manager.h" +#include "application_manager/commands/command.h" namespace application_manager { +CREATE_LOGGERPTR_GLOBAL(logger_, "ApplicationManager") + CommandHolderImpl::CommandHolderImpl(ApplicationManager& app_manager) : app_manager_(app_manager) {} void CommandHolderImpl::Suspend( - const std::string& policy_app_id, + ApplicationSharedPtr application, + CommandType type, utils::SharedPtr command) { + LOG4CXX_AUTO_TRACE(logger_); + LOG4CXX_DEBUG(logger_, "Suspending command(s) for application: " + << application->policy_app_id()); + sync_primitives::AutoLock lock(commands_lock_); + + if (CommandType::kHmiCommand == type) { + app_hmi_commands_[application].push_back(command); + LOG4CXX_DEBUG(logger_, "Suspended HMI command(s): " + << app_hmi_commands_.size()); + } else { + app_mobile_commands_[application].push_back(command); + LOG4CXX_DEBUG(logger_, "Suspended mobile command(s): " + << app_hmi_commands_.size()); + } +} + +void CommandHolderImpl::Resume(ApplicationSharedPtr application, + CommandType type) { + LOG4CXX_AUTO_TRACE(logger_); + LOG4CXX_DEBUG(logger_, "Resuming command(s) for application: " + << application->policy_app_id()); + if (CommandType::kHmiCommand == type) { + ResumeHmiCommand(application); + } else { + ResumeMobileCommand(application); + } +} + +void CommandHolderImpl::Clear(ApplicationSharedPtr application) { + LOG4CXX_AUTO_TRACE(logger_); + LOG4CXX_DEBUG(logger_, "Clearing command(s) for application: " + << application->policy_app_id()); sync_primitives::AutoLock lock(commands_lock_); - app_commands_[policy_app_id].push_back(command); + auto app_hmi_commands = app_hmi_commands_.find(application); + if (app_hmi_commands_.end() != app_hmi_commands) { + LOG4CXX_DEBUG(logger_, "Clearing HMI command(s): " + << app_hmi_commands->second.size()); + app_hmi_commands_.erase(app_hmi_commands); + } + + auto app_mobile_commands = app_mobile_commands_.find(application); + if (app_mobile_commands_.end() != app_mobile_commands) { + LOG4CXX_DEBUG(logger_, "Clearing mobile command(s): " + << app_mobile_commands->second.size()); + app_mobile_commands_.erase(app_mobile_commands); + } } -void CommandHolderImpl::Resume(const std::string& policy_app_id) { +void CommandHolderImpl::ResumeHmiCommand(ApplicationSharedPtr application) { sync_primitives::AutoLock lock(commands_lock_); - auto app_commands = app_commands_.find(policy_app_id); - if (app_commands_.end() == app_commands) { + auto app_commands = app_hmi_commands_.find(application); + if (app_hmi_commands_.end() == app_commands) { return; } + + LOG4CXX_DEBUG(logger_, "Resuming HMI command(s): " + << app_hmi_commands_.size()); + for (auto cmd : app_commands->second) { + (*cmd)[strings::msg_params][strings::app_id] = application->hmi_app_id(); app_manager_.ManageHMICommand(cmd); } - app_commands_.erase(app_commands); + app_hmi_commands_.erase(app_commands); } -void CommandHolderImpl::Clear(const std::string& policy_app_id) { +void CommandHolderImpl::ResumeMobileCommand(ApplicationSharedPtr application) { sync_primitives::AutoLock lock(commands_lock_); - auto app_commands = app_commands_.find(policy_app_id); - if (app_commands_.end() == app_commands) { + auto app_commands = app_mobile_commands_.find(application); + if (app_mobile_commands_.end() == app_commands) { return; } - app_commands_.erase(app_commands); + LOG4CXX_DEBUG(logger_, "Resuming mobile command(s): " + << app_mobile_commands_.size()); + + for (auto cmd : app_commands->second) { + (*cmd)[strings::params][strings::connection_key] = + application->app_id(); + app_manager_.ManageMobileCommand( + cmd, + commands::Command::CommandOrigin::ORIGIN_MOBILE); + } + + app_mobile_commands_.erase(app_commands); } } // application_manager diff --git a/src/components/application_manager/test/application_manager_impl_mock_hmi_test.cc b/src/components/application_manager/test/application_manager_impl_mock_hmi_test.cc index 7ca2ff9d98..4de7be7c4c 100644 --- a/src/components/application_manager/test/application_manager_impl_mock_hmi_test.cc +++ b/src/components/application_manager/test/application_manager_impl_mock_hmi_test.cc @@ -202,6 +202,10 @@ TEST_F(ApplicationManagerImplMockHmiTest, commands::MessageSharedPtr hmi_msg_3 = utils::MakeShared(); + (*hmi_msg_1)[strings::msg_params][strings::app_id] = + (*hmi_msg_2)[strings::msg_params][strings::app_id] = + (*hmi_msg_3)[strings::msg_params][strings::app_id] = application_id; + EXPECT_CALL(*cmd_1, Init()).Times(0); EXPECT_CALL(*cmd_2, Init()).Times(0); EXPECT_CALL(*cmd_3, Init()).Times(0); diff --git a/src/components/application_manager/test/command_holder_test.cc b/src/components/application_manager/test/command_holder_test.cc index 8082c3811f..e10cd5d008 100644 --- a/src/components/application_manager/test/command_holder_test.cc +++ b/src/components/application_manager/test/command_holder_test.cc @@ -33,36 +33,56 @@ #include #include "application_manager/command_holder_impl.h" +#include "application_manager/commands/command.h" #include "smart_objects/smart_object.h" #include "utils/shared_ptr.h" +#include "utils/make_shared.h" #include "application_manager/mock_application_manager.h" +#include "application_manager/mock_application.h" namespace test { namespace components { namespace application_manager_test { using testing::_; +using testing::Return; namespace am = application_manager; class CommandHolderImplTest : public testing::Test { public: CommandHolderImplTest() - : cmd_ptr_(new smart_objects::SmartObject), kPolicyAppId_("p_app_id") {} + : kPolicyAppId_("p_app_id") + , kHmiApplicationId_(123) + , kConnectionKey_(56789) + , cmd_ptr_(new smart_objects::SmartObject) + , mock_app_ptr_(new MockApplication) {} + + void SetUp() OVERRIDE { + ON_CALL(*mock_app_ptr_, app_id()).WillByDefault(Return(kConnectionKey_)); + ON_CALL(*mock_app_ptr_, hmi_app_id()) + .WillByDefault(Return(kHmiApplicationId_)); + ON_CALL(*mock_app_ptr_, policy_app_id()) + .WillByDefault(Return(kPolicyAppId_)); + } MockApplicationManager mock_app_manager_; - utils::SharedPtr cmd_ptr_; const std::string kPolicyAppId_; + const uint32_t kHmiApplicationId_; + const uint32_t kConnectionKey_; + utils::SharedPtr cmd_ptr_; + utils::SharedPtr mock_app_ptr_; }; TEST_F(CommandHolderImplTest, HoldOne_ExpectReleaseOne) { am::CommandHolderImpl cmd_holder(mock_app_manager_); - cmd_holder.Suspend(kPolicyAppId_, cmd_ptr_); + cmd_holder.Suspend( + mock_app_ptr_, am::CommandHolder::CommandType::kHmiCommand, cmd_ptr_); // Act EXPECT_CALL(mock_app_manager_, ManageHMICommand(cmd_ptr_)); - cmd_holder.Resume(kPolicyAppId_); + cmd_holder.Resume(mock_app_ptr_, am::CommandHolder::CommandType::kHmiCommand); } TEST_F(CommandHolderImplTest, HoldMany_ExpectReleaseSame) { @@ -70,34 +90,42 @@ TEST_F(CommandHolderImplTest, HoldMany_ExpectReleaseSame) { int32_t iterations = 0; do { - cmd_holder.Suspend(kPolicyAppId_, cmd_ptr_); + cmd_holder.Suspend( + mock_app_ptr_, am::CommandHolder::CommandType::kHmiCommand, cmd_ptr_); ++iterations; } while (iterations < 5); // Act EXPECT_CALL(mock_app_manager_, ManageHMICommand(cmd_ptr_)).Times(iterations); - cmd_holder.Resume(kPolicyAppId_); + cmd_holder.Resume(mock_app_ptr_, am::CommandHolder::CommandType::kHmiCommand); } TEST_F(CommandHolderImplTest, Hold_Drop_ExpectNoReleased) { am::CommandHolderImpl cmd_holder(mock_app_manager_); - cmd_holder.Suspend(kPolicyAppId_, cmd_ptr_); - cmd_holder.Suspend(kPolicyAppId_, cmd_ptr_); + cmd_holder.Suspend( + mock_app_ptr_, am::CommandHolder::CommandType::kHmiCommand, cmd_ptr_); + cmd_holder.Suspend( + mock_app_ptr_, am::CommandHolder::CommandType::kHmiCommand, cmd_ptr_); // Act - cmd_holder.Clear(kPolicyAppId_); + cmd_holder.Clear(mock_app_ptr_); EXPECT_CALL(mock_app_manager_, ManageHMICommand(cmd_ptr_)).Times(0); - cmd_holder.Resume(kPolicyAppId_); + cmd_holder.Resume(mock_app_ptr_, am::CommandHolder::CommandType::kHmiCommand); } TEST_F(CommandHolderImplTest, Hold_ReleaseAnotherId_ExpectNoReleased) { am::CommandHolderImpl cmd_holder(mock_app_manager_); - cmd_holder.Suspend(kPolicyAppId_, cmd_ptr_); - cmd_holder.Suspend(kPolicyAppId_, cmd_ptr_); + cmd_holder.Suspend( + mock_app_ptr_, am::CommandHolder::CommandType::kHmiCommand, cmd_ptr_); + cmd_holder.Suspend( + mock_app_ptr_, am::CommandHolder::CommandType::kHmiCommand, cmd_ptr_); // Act + utils::SharedPtr another_app = + utils::MakeShared(); + EXPECT_CALL(mock_app_manager_, ManageHMICommand(cmd_ptr_)).Times(0); - cmd_holder.Resume("anotherId"); + cmd_holder.Resume(another_app, am::CommandHolder::CommandType::kHmiCommand); } TEST_F(CommandHolderImplTest, Hold_DropAnotherId_ExpectReleased) { @@ -105,15 +133,39 @@ TEST_F(CommandHolderImplTest, Hold_DropAnotherId_ExpectReleased) { int32_t iterations = 0; do { - cmd_holder.Suspend(kPolicyAppId_, cmd_ptr_); + cmd_holder.Suspend( + mock_app_ptr_, am::CommandHolder::CommandType::kHmiCommand, cmd_ptr_); ++iterations; } while (iterations < 3); // Act - cmd_holder.Clear("anotherId"); + utils::SharedPtr another_app = + utils::MakeShared(); + cmd_holder.Clear(another_app); EXPECT_CALL(mock_app_manager_, ManageHMICommand(cmd_ptr_)).Times(iterations); - cmd_holder.Resume(kPolicyAppId_); + cmd_holder.Resume(mock_app_ptr_, am::CommandHolder::CommandType::kHmiCommand); +} + +TEST_F(CommandHolderImplTest, Hold_Mobile_and_HMI_commands_ExpectReleased) { + am::CommandHolderImpl cmd_holder(mock_app_manager_); + + cmd_holder.Suspend( + mock_app_ptr_, am::CommandHolder::CommandType::kHmiCommand, cmd_ptr_); + + cmd_holder.Suspend( + mock_app_ptr_, am::CommandHolder::CommandType::kMobileCommand, cmd_ptr_); + + // Act + EXPECT_CALL(mock_app_manager_, ManageHMICommand(cmd_ptr_)); + cmd_holder.Resume(mock_app_ptr_, am::CommandHolder::CommandType::kHmiCommand); + + EXPECT_CALL( + mock_app_manager_, + ManageMobileCommand(cmd_ptr_, + am::commands::Command::CommandOrigin::ORIGIN_MOBILE)); + cmd_holder.Resume(mock_app_ptr_, + am::CommandHolder::CommandType::kMobileCommand); } } // application_manager_test -- cgit v1.2.1