From 0d53e41272c5da5e3c73ab9f3121cb95603f68fe Mon Sep 17 00:00:00 2001 From: "Yana Chernysheva (GitHub)" <59469418+ychernysheva@users.noreply.github.com> Date: Tue, 6 Oct 2020 00:11:43 +0300 Subject: Add check for already processed resumptions (#3525) --- .../resumption/resumption_data_processor_impl.h | 45 +++---- .../resumption/resumption_data_processor_impl.cc | 139 ++++++--------------- .../test/resumption/resume_ctrl_test.cc | 19 --- 3 files changed, 61 insertions(+), 142 deletions(-) diff --git a/src/components/application_manager/include/application_manager/resumption/resumption_data_processor_impl.h b/src/components/application_manager/include/application_manager/resumption/resumption_data_processor_impl.h index 12d25e2ac5..c2040852ca 100644 --- a/src/components/application_manager/include/application_manager/resumption/resumption_data_processor_impl.h +++ b/src/components/application_manager/include/application_manager/resumption/resumption_data_processor_impl.h @@ -125,14 +125,22 @@ class ResumptionDataProcessorImpl const ResumptionRequest& found_request); /** - * @brief IsResumptionFinished checks whether some responses are still waiting + * @brief EraseProcessedRequest erases processed request from list of pending + * requests * @param app_id ID of application, related to event * @param found_request reference to found request + */ + void EraseProcessedRequest(const uint32_t app_id, + const ResumptionRequest& found_request); + + /** + * @brief IsResumptionFinished checks whether some responses are still + * waiting + * @param app_id ID of application, related to event * @return true, if resumption for this application is finished, or false, if - * some requests aren't processed yet + * some requests aren't processed yet */ - bool IsResumptionFinished(const uint32_t app_id, - const ResumptionRequest& found_request); + bool IsResumptionFinished(const uint32_t app_id) const; /** * @brief IsResumptionSuccessful checks whether overall resumption status @@ -156,12 +164,8 @@ class ResumptionDataProcessorImpl * @brief EraseAppResumptionData erases data, needed for resumption, for * given application * @param app_id ID of application, related to event - * @param function_id Function ID - * @param corr_id Correlation ID */ - void EraseAppResumptionData(const uint32_t app_id, - const hmi_apis::FunctionID::eType function_id, - const int32_t corr_id); + void EraseAppResumptionData(const uint32_t app_id); /** * @brief Processes response message from HMI @@ -173,6 +177,14 @@ class ResumptionDataProcessorImpl const hmi_apis::FunctionID::eType function_id, const int32_t corr_id); + /** + * @brief Checks whether resumption is successful and finalizes resumption + * corresponding to result of this check + * @param callback Function to be called when data resumption will be finished + * @param app_id ID of application, related to event + */ + void FinalizeResumption(const ResumeCtrl::ResumptionCallBack& callback, + const uint32_t app_id); /** * @brief Revert the data to the state before Resumption * @param shared ptr to application @@ -355,34 +367,25 @@ class ResumptionDataProcessorImpl const smart_objects::SmartObject& request, const smart_objects::SmartObject& response) const; - /** - * @brief Determines whether application has saved data, including - * submenus, commands, choice sets, global properties, subscriptions to - * restore. - * @param saved_app smart object containing saved app data - * @return bool value stating whether app has mentioned data to restore - */ - bool HasDataToRestore(const smart_objects::SmartObject& saved_app) const; - app_mngr::ApplicationManager& application_manager_; /** * @brief A map of the IDs and Application Resumption Status for these ID **/ std::map resumption_status_; - sync_primitives::RWLock resumption_status_lock_; + mutable sync_primitives::RWLock resumption_status_lock_; /** * @brief A map of callbacks used when resumption is finished */ std::map register_callbacks_; - sync_primitives::RWLock register_callbacks_lock_; + mutable sync_primitives::RWLock register_callbacks_lock_; /** * @brief A map of sent requests and corresponding app_id */ std::map request_app_ids_; - sync_primitives::RWLock request_app_ids_lock_; + mutable sync_primitives::RWLock request_app_ids_lock_; }; } // namespace resumption diff --git a/src/components/application_manager/src/resumption/resumption_data_processor_impl.cc b/src/components/application_manager/src/resumption/resumption_data_processor_impl.cc index d3ac9c24e1..792e7f97ea 100644 --- a/src/components/application_manager/src/resumption/resumption_data_processor_impl.cc +++ b/src/components/application_manager/src/resumption/resumption_data_processor_impl.cc @@ -71,12 +71,6 @@ void ResumptionDataProcessorImpl::Restore( ResumeCtrl::ResumptionCallBack callback) { SDL_LOG_AUTO_TRACE(); - if (!HasDataToRestore(saved_app)) { - SDL_LOG_DEBUG("No data to restore, resumption is successful"); - callback(mobile_apis::Result::SUCCESS, "Data resumption succesful"); - return; - } - AddFiles(application, saved_app); AddSubmenus(application, saved_app); AddCommands(application, saved_app); @@ -85,99 +79,15 @@ void ResumptionDataProcessorImpl::Restore( AddSubscriptions(application, saved_app); AddWindows(application, saved_app); - resumption_status_lock_.AcquireForReading(); const auto app_id = application->app_id(); - bool is_requests_list_empty = true; - if (resumption_status_.find(app_id) != resumption_status_.end()) { - is_requests_list_empty = - resumption_status_[app_id].list_of_sent_requests.empty(); - } - resumption_status_lock_.Release(); - - if (!is_requests_list_empty) { + if (!IsResumptionFinished(app_id)) { sync_primitives::AutoWriteLock lock(register_callbacks_lock_); register_callbacks_[app_id] = callback; } else { - SDL_LOG_DEBUG("No requests to HMI for " << app_id - << " , resumption is successful"); - callback(mobile_apis::Result::SUCCESS, "Data resumption successful"); + FinalizeResumption(callback, app_id); } } -bool ResumptionDataProcessorImpl::HasDataToRestore( - const smart_objects::SmartObject& saved_app) const { - SDL_LOG_AUTO_TRACE(); - - auto has_data_to_restore = [&saved_app]() -> bool { - return !saved_app[strings::application_files].empty() || - !saved_app[strings::application_submenus].empty() || - !saved_app[strings::application_commands].empty() || - !saved_app[strings::application_choice_sets].empty() || - !saved_app[strings::windows_info].empty(); - }; - - auto has_gp_to_restore = [&saved_app]() -> bool { - const smart_objects::SmartObject& global_properties = - saved_app[strings::application_global_properties]; - - return !global_properties[strings::help_prompt].empty() || - !global_properties[strings::keyboard_properties].empty() || - !global_properties[strings::menu_icon].empty() || - !global_properties[strings::menu_title].empty() || - !global_properties[strings::timeout_prompt].empty() || - !global_properties[strings::vr_help].empty() || - !global_properties[strings::vr_help_title].empty(); - }; - - auto has_subscriptions_to_restore = [&saved_app]() -> bool { - const smart_objects::SmartObject& subscriptions = - saved_app[strings::application_subscriptions]; - - const bool has_ivi_subscriptions = - !subscriptions[strings::application_vehicle_info].empty(); - - const bool has_button_subscriptions = - !subscriptions[strings::application_buttons].empty() && - !(subscriptions[strings::application_buttons].length() == 1 && - static_cast( - subscriptions[strings::application_buttons][0].asInt()) == - hmi_apis::Common_ButtonName::CUSTOM_BUTTON); - - const bool has_waypoints_subscriptions = - subscriptions[strings::subscribed_for_way_points].asBool(); - - const bool has_appservice_subscriptions = - subscriptions.keyExists(app_mngr::hmi_interface::app_service) && - !subscriptions[app_mngr::hmi_interface::app_service].empty(); - - const bool has_system_capability_subscriptions = - subscriptions.keyExists(strings::system_capability) && - !subscriptions[strings::system_capability].empty(); - - return has_ivi_subscriptions || has_button_subscriptions || - has_waypoints_subscriptions || has_appservice_subscriptions || - has_system_capability_subscriptions; - }; - - if (has_data_to_restore()) { - SDL_LOG_DEBUG("Application has data to restore"); - return true; - } - - if (has_gp_to_restore()) { - SDL_LOG_DEBUG("Application has global properties to restore"); - return true; - } - - if (has_subscriptions_to_restore()) { - SDL_LOG_DEBUG("Application has subscriptions to restore"); - return true; - } - - SDL_LOG_DEBUG("Application does not have any data to restore"); - return false; -} - utils::Optional ResumptionDataProcessorImpl::GetAppIdWaitingForResponse( const hmi_apis::FunctionID::eType function_id, const int32_t corr_id) { @@ -260,7 +170,7 @@ void ResumptionDataProcessorImpl::ProcessResumptionStatus( } } -bool ResumptionDataProcessorImpl::IsResumptionFinished( +void ResumptionDataProcessorImpl::EraseProcessedRequest( const uint32_t app_id, const ResumptionRequest& found_request) { SDL_LOG_AUTO_TRACE(); @@ -277,8 +187,19 @@ bool ResumptionDataProcessorImpl::IsResumptionFinished( found_request.request_id.function_id; }); list_of_sent_requests.erase(request_iter); +} + +bool ResumptionDataProcessorImpl::IsResumptionFinished( + const uint32_t app_id) const { + SDL_LOG_AUTO_TRACE(); - return list_of_sent_requests.empty(); + sync_primitives::AutoReadLock lock(resumption_status_lock_); + bool is_requests_list_empty = true; + const auto app_status = resumption_status_.find(app_id); + if (app_status != resumption_status_.end()) { + is_requests_list_empty = app_status->second.list_of_sent_requests.empty(); + } + return is_requests_list_empty; } utils::Optional @@ -299,7 +220,7 @@ bool ResumptionDataProcessorImpl::IsResumptionSuccessful( sync_primitives::AutoReadLock lock(resumption_status_lock_); auto it = resumption_status_.find(app_id); if (resumption_status_.end() == it) { - return false; + return true; } const ApplicationResumptionStatus& status = it->second; @@ -309,17 +230,27 @@ bool ResumptionDataProcessorImpl::IsResumptionSuccessful( } void ResumptionDataProcessorImpl::EraseAppResumptionData( - const uint32_t app_id, - const hmi_apis::FunctionID::eType function_id, - const int32_t corr_id) { + const uint32_t app_id) { SDL_LOG_AUTO_TRACE(); + std::vector all_requests; + resumption_status_lock_.AcquireForWriting(); + all_requests.insert(all_requests.end(), + resumption_status_[app_id].successful_requests.begin(), + resumption_status_[app_id].successful_requests.end()); + all_requests.insert(all_requests.end(), + resumption_status_[app_id].error_requests.begin(), + resumption_status_[app_id].error_requests.end()); + resumption_status_.erase(app_id); resumption_status_lock_.Release(); request_app_ids_lock_.AcquireForWriting(); - request_app_ids_.erase({function_id, corr_id}); + for (auto request : all_requests) { + request_app_ids_.erase( + {request.request_id.function_id, request.request_id.correlation_id}); + } request_app_ids_lock_.Release(); register_callbacks_lock_.AcquireForWriting(); @@ -354,8 +285,9 @@ void ResumptionDataProcessorImpl::ProcessResponseFromHMI( auto request = *found_request; ProcessResumptionStatus(app_id, response, request); + EraseProcessedRequest(app_id, request); - if (!IsResumptionFinished(app_id, request)) { + if (!IsResumptionFinished(app_id)) { SDL_LOG_DEBUG("Resumption app " << app_id << " not finished. Some requests are still waited"); return; @@ -367,7 +299,11 @@ void ResumptionDataProcessorImpl::ProcessResponseFromHMI( return; } auto callback = *found_callback; + FinalizeResumption(callback, app_id); +} +void ResumptionDataProcessorImpl::FinalizeResumption( + const ResumeCtrl::ResumptionCallBack& callback, const uint32_t app_id) { if (IsResumptionSuccessful(app_id)) { SDL_LOG_DEBUG("Resumption for app " << app_id << " successful"); callback(mobile_apis::Result::SUCCESS, "Data resumption successful"); @@ -378,8 +314,7 @@ void ResumptionDataProcessorImpl::ProcessResponseFromHMI( RevertRestoredData(application_manager_.application(app_id)); application_manager_.state_controller().DropPostponedWindows(app_id); } - - EraseAppResumptionData(app_id, function_id, corr_id); + EraseAppResumptionData(app_id); } void ResumptionDataProcessorImpl::HandleOnTimeOut( diff --git a/src/components/application_manager/test/resumption/resume_ctrl_test.cc b/src/components/application_manager/test/resumption/resume_ctrl_test.cc index ded6aa956e..3953e7e2dc 100644 --- a/src/components/application_manager/test/resumption/resume_ctrl_test.cc +++ b/src/components/application_manager/test/resumption/resume_ctrl_test.cc @@ -251,25 +251,6 @@ class ResumeCtrlTest : public ::testing::Test { * @brief Group of tests which check starting resumption with different data */ -TEST_F(ResumeCtrlTest, StartResumption_AppWithGrammarId) { - smart_objects::SmartObject saved_app; - saved_app[application_manager::strings::hash_id] = kHash_; - saved_app[application_manager::strings::grammar_id] = kTestGrammarId_; - - // Check RestoreApplicationData - GetInfoFromApp(); - ON_CALL(mock_app_mngr_, GetDefaultHmiLevel(const_app_)) - .WillByDefault(Return(kDefaultTestLevel_)); - ON_CALL(*mock_storage_, - GetSavedApplication(kTestPolicyAppId_, kMacAddress_, _)) - .WillByDefault(DoAll(SetArgReferee<2>(saved_app), Return(true))); - - EXPECT_CALL(*mock_app_, set_grammar_id(kTestGrammarId_)); - - const bool res = res_ctrl_->StartResumption(mock_app_, kHash_, callback_); - EXPECT_TRUE(res); -} - MATCHER_P4(CheckAppFile, is_persistent, is_download, file_name, file_type, "") { application_manager::AppFile app_file = arg; return app_file.is_persistent == is_persistent && -- cgit v1.2.1