diff --git a/src/engine/BUILD.bazel b/src/engine/BUILD.bazel index e1af3a6d3..84f73dccf 100644 --- a/src/engine/BUILD.bazel +++ b/src/engine/BUILD.bazel @@ -194,15 +194,18 @@ mozc_cc_test( srcs = ["engine_test.cc"], deps = [ ":engine", + ":engine_builder", ":modules", ":spellchecker_interface", "//base:logging", + "//base:thread", "//composer:query", "//converter:segments", "//data_manager/testing:mock_data_manager", "//testing:gunit_main", "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", + "@com_google_absl//absl/time", ], ) diff --git a/src/engine/engine.cc b/src/engine/engine.cc index c8825b46b..a9d491607 100644 --- a/src/engine/engine.cc +++ b/src/engine/engine.cc @@ -265,11 +265,11 @@ bool Engine::MaybeReloadEngine(EngineReloadResponse *response) { // Maybe build new engine if new request is received. // EngineBuilder::Build just returns a future object so // client needs to replace the new engine when the future is the ready to use. - if (!engine_response_future_ && current_engine_id_ != latest_engine_id_ && - latest_engine_id_ != 0) { - engine_response_future_ = loader_->Build(latest_engine_id_); + if (!engine_response_future_ && current_data_id_ != latest_data_id_ && + latest_data_id_ != 0) { + engine_response_future_ = loader_->Build(latest_data_id_); // Wait the engine if the no new engine is loaded so far. - if (current_engine_id_ == 0 || always_wait_for_engine_response_future_) { + if (current_data_id_ == 0 || always_wait_for_engine_response_future_) { engine_response_future_->Wait(); } } @@ -296,7 +296,7 @@ bool Engine::MaybeReloadEngine(EngineReloadResponse *response) { loader_->UnregisterRequest(engine_response.id); // Update latest_engine_id_ if latest_engine_id_ == engine_response.id. // Otherwise, latest_engine_id_ may already be updated by the new request. - latest_engine_id_.compare_exchange_strong(engine_response.id, rollback_id); + latest_data_id_.compare_exchange_strong(engine_response.id, rollback_id); return false; } @@ -315,7 +315,7 @@ bool Engine::MaybeReloadEngine(EngineReloadResponse *response) { return false; } - current_engine_id_ = engine_response.id; + current_data_id_ = engine_response.id; response->set_status(EngineReloadResponse::RELOADED); return true; } @@ -324,7 +324,7 @@ bool Engine::SendEngineReloadRequest(const EngineReloadRequest &request) { if (!loader_) { return false; } - latest_engine_id_ = loader_->RegisterRequest(request); + latest_data_id_ = loader_->RegisterRequest(request); return true; } diff --git a/src/engine/engine.h b/src/engine/engine.h index 1cc92dbdc..58af6ea1d 100644 --- a/src/engine/engine.h +++ b/src/engine/engine.h @@ -162,8 +162,8 @@ class Engine : public EngineInterface { std::unique_ptr converter_; std::unique_ptr user_data_manager_; - std::atomic latest_engine_id_ = 0; - std::atomic current_engine_id_ = 0; + std::atomic latest_data_id_ = 0; + std::atomic current_data_id_ = 0; std::unique_ptr engine_response_future_; // used only in unittest to perform blocking behavior. bool always_wait_for_engine_response_future_ = false; diff --git a/src/engine/engine_test.cc b/src/engine/engine_test.cc index 107f494bf..2b3d1f5c1 100644 --- a/src/engine/engine_test.cc +++ b/src/engine/engine_test.cc @@ -29,6 +29,7 @@ #include "engine/engine.h" +#include #include #include #include @@ -36,10 +37,14 @@ #include "absl/status/statusor.h" #include "absl/strings/string_view.h" +#include "absl/time/clock.h" +#include "absl/time/time.h" #include "base/logging.h" +#include "base/thread.h" #include "composer/query.h" #include "converter/segments.h" #include "data_manager/testing/mock_data_manager.h" +#include "engine/data_loader.h" #include "engine/modules.h" #include "engine/spellchecker_interface.h" #include "testing/gmock.h" @@ -49,6 +54,11 @@ namespace mozc { namespace engine { namespace { + +using ::testing::_; +using ::testing::InSequence; +using ::testing::Return; + class MockSpellchecker : public engine::SpellcheckerInterface { public: MOCK_METHOD(commands::CheckSpellingResponse, CheckSpelling, @@ -60,6 +70,15 @@ class MockSpellchecker : public engine::SpellcheckerInterface { MOCK_METHOD(void, MaybeApplyHomonymCorrection, (Segments *), (const override)); }; + +class MockDataLoader : public DataLoader { + public: + MOCK_METHOD(uint64_t, RegisterRequest, (const EngineReloadRequest &), + (override)); + MOCK_METHOD(uint64_t, UnregisterRequest, (uint64_t), (override)); + MOCK_METHOD(std::unique_ptr, Build, (uint64_t), + (const override)); +}; } // namespace TEST(EngineTest, ReloadModulesTest) { @@ -82,5 +101,238 @@ TEST(EngineTest, ReloadModulesTest) { EXPECT_EQ(engine->GetModulesForTesting()->GetSpellchecker(), &spellchecker); } +// Tests the interaction with DataLoader for successful Engine +// reload event. +TEST(EngineTest, DataLoadSuccessfulScenarioTest) { + auto data_manager = std::make_unique(); + absl::string_view data_version = "DataLoadSuccessfulScenarioTest"; + EXPECT_CALL(*data_manager, GetDataVersion()) + .WillRepeatedly(Return(data_version)); + auto modules = std::make_unique(); + CHECK_OK(modules->Init(std::move(data_manager))); + + auto data_loader = std::make_unique(); + EXPECT_CALL(*data_loader, RegisterRequest(_)).WillRepeatedly(Return(1)); + EXPECT_CALL(*data_loader, Build(1)) + .WillOnce(Return( + std::make_unique>([&]() { + // takes 0.1 seconds to make engine. + absl::SleepFor(absl::Milliseconds(100)); + DataLoader::Response result; + result.id = 1; + result.response.set_status(EngineReloadResponse::RELOAD_READY); + result.modules = std::move(modules); + return result; + }))); + + absl::StatusOr> engine_status = + Engine::CreateMobileEngine(std::make_unique()); + EXPECT_OK(engine_status); + + Engine &engine = *engine_status.value(); + engine.SetDataLoaderForTesting(std::move(data_loader)); + + EngineReloadRequest request; + request.set_engine_type(EngineReloadRequest::MOBILE); + request.set_file_path("placeholder"); // OK for MockDataLoader + + EXPECT_TRUE(engine.SendEngineReloadRequest(request)); + + EngineReloadResponse response; + EXPECT_TRUE(engine.MaybeReloadEngine(&response)); + + // When the engine is created first, we wait until the engine gets ready. + EXPECT_EQ(engine.GetDataVersion(), data_version); + + // New session is created, but Build is not called as the id is the same. + EXPECT_TRUE(engine.SendEngineReloadRequest(request)); + EXPECT_FALSE(engine.MaybeReloadEngine(&response)); + EXPECT_EQ(engine.GetDataVersion(), data_version); +} + +// Tests situations to handle multiple new requests. +TEST(EngineTest, DataUpdateSuccessfulScenarioTest) { + auto data_loader = std::make_unique(); + MockDataLoader *data_loader_ptr = data_loader.get(); + + auto data_manager1 = std::make_unique(); + absl::string_view data_version1 = "EngineUpdateSuccessfulScenarioTest_1"; + EXPECT_CALL(*data_manager1, GetDataVersion()) + .WillRepeatedly(Return(data_version1)); + auto modules1 = std::make_unique(); + CHECK_OK(modules1->Init(std::move(data_manager1))); + + auto data_manager2 = std::make_unique(); + absl::string_view data_version2 = "EngineUpdateSuccessfulScenarioTest_2"; + EXPECT_CALL(*data_manager2, GetDataVersion()) + .WillRepeatedly(Return(data_version2)); + auto modules2 = std::make_unique(); + CHECK_OK(modules2->Init(std::move(data_manager2))); + + InSequence seq; // EXPECT_CALL is called sequentially. + + EXPECT_CALL(*data_loader, RegisterRequest(_)).WillRepeatedly(Return(1)); + + EXPECT_CALL(*data_loader, Build(1)) + .WillOnce(Return( + std::make_unique>([&]() { + absl::SleepFor(absl::Milliseconds(100)); + DataLoader::Response result; + result.id = 1; + result.response.set_status(EngineReloadResponse::RELOAD_READY); + result.modules = std::move(modules1); + return result; + }))); + + absl::StatusOr> engine_status = + Engine::CreateMobileEngine(std::make_unique()); + EXPECT_OK(engine_status); + + Engine &engine = *engine_status.value(); + engine.SetDataLoaderForTesting(std::move(data_loader)); + engine.SetAlwaysWaitForEngineResponseFutureForTesting(true); + + EngineReloadRequest request; + request.set_engine_type(EngineReloadRequest::MOBILE); + request.set_file_path("placeholder"); // OK for MockDataLoader + + // Send a request, and get a response with id=1. + EXPECT_TRUE(engine.SendEngineReloadRequest(request)); + + EngineReloadResponse response; + EXPECT_TRUE(engine.MaybeReloadEngine(&response)); + + EXPECT_EQ(engine.GetDataVersion(), data_version1); + + // Use data_loader_ptr after std::move(engine_build). + EXPECT_CALL(*data_loader_ptr, RegisterRequest(_)) + .WillRepeatedly(Return(2)); + + EXPECT_CALL(*data_loader_ptr, Build(2)) + .WillOnce(Return( + std::make_unique>([&]() { + absl::SleepFor(absl::Milliseconds(100)); + DataLoader::Response result; + result.id = 2; + result.response.set_status(EngineReloadResponse::RELOAD_READY); + result.modules = std::move(modules2); + return result; + }))); + + // Send a request, and get a response with id=2. + EXPECT_TRUE(engine.SendEngineReloadRequest(request)); + EXPECT_TRUE(engine.MaybeReloadEngine(&response)); + EXPECT_EQ(engine.GetDataVersion(), data_version2); +} + +// Tests the interaction with DataLoader in the situation where +// requested data is broken. +TEST(EngineTest, ReloadInvalidDataTest) { + auto data_loader = std::make_unique(); + MockDataLoader *data_loader_ptr = data_loader.get(); + + InSequence seq; // EXPECT_CALL is called sequentially. + + absl::StatusOr> engine_status = + Engine::CreateMobileEngine(std::make_unique()); + EXPECT_OK(engine_status); + + Engine &engine = *engine_status.value(); + engine.SetDataLoaderForTesting(std::move(data_loader)); + + EXPECT_CALL(*data_loader_ptr, RegisterRequest(_)).WillOnce(Return(1)); + + EngineReloadRequest request; + request.set_engine_type(EngineReloadRequest::MOBILE); + request.set_file_path("placeholder"); // OK for MockDataLoader + EXPECT_TRUE(engine.SendEngineReloadRequest(request)); + + EXPECT_CALL(*data_loader_ptr, Build(1)) + .WillOnce(Return( + std::make_unique>([&]() { + absl::SleepFor(absl::Milliseconds(100)); + DataLoader::Response result; + result.id = 1; + result.response.set_status(EngineReloadResponse::DATA_BROKEN); + return result; + }))); + EXPECT_CALL(*data_loader_ptr, UnregisterRequest(1)).WillOnce(Return(0)); + + // Build() is called, but it returns invalid data, so new data is not used. + EngineReloadResponse response; + EXPECT_FALSE(engine.MaybeReloadEngine(&response)); + + // Sends the same request again, but the request is already marked as + // unregistered. + EXPECT_CALL(*data_loader_ptr, RegisterRequest(_)).WillOnce(Return(0)); + EXPECT_TRUE(engine.SendEngineReloadRequest(request)); + EXPECT_FALSE(engine.MaybeReloadEngine(&response)); +} + +// Tests the rollback scenario +TEST(EngineTest, RollbackDataTest) { + auto data_loader = std::make_unique(); + MockDataLoader *data_loader_ptr = data_loader.get(); + + InSequence seq; // EXPECT_CALL is called sequentially. + + auto data_manager = std::make_unique(); + testing::MockDataManager *data_manager_ptr = data_manager.get(); + absl::string_view data_version = "EngineRollbackDataTest"; + auto modules = std::make_unique(); + CHECK_OK(modules->Init(std::move(data_manager))); + + absl::StatusOr> engine_status = + Engine::CreateMobileEngine(std::make_unique()); + EXPECT_OK(engine_status); + Engine &engine = *engine_status.value(); + + engine.SetDataLoaderForTesting(std::move(data_loader)); + engine.SetAlwaysWaitForEngineResponseFutureForTesting(true); + + EngineReloadRequest request; + request.set_engine_type(EngineReloadRequest::MOBILE); + request.set_file_path("placeholder"); // OK for MockDataLoader + + // Sends multiple requests three times. 1 -> 2 -> 3. + // 3 is the latest id. + for (int eid = 1; eid <= 3; ++eid) { + EXPECT_CALL(*data_loader_ptr, RegisterRequest(_)).WillOnce(Return(eid)); + ASSERT_TRUE(engine.SendEngineReloadRequest(request)); + } + + for (int eid = 3; eid >= 1; --eid) { + // Rollback as 3 -> 2 -> 1. 1 is only valid engine. + EXPECT_CALL(*data_loader_ptr, Build(eid)) + .WillOnce(Return( + std::make_unique>([&]() { + absl::SleepFor(absl::Milliseconds(100)); + DataLoader::Response result; + result.id = eid; + if (eid == 1) { + result.response.set_status(EngineReloadResponse::RELOAD_READY); + result.modules = std::move(modules); + } else { + result.response.set_status(EngineReloadResponse::DATA_BROKEN); + } + return result; + }))); + // Engine of 3, and 2 are unregistered. + // The second best id (2, and 1) are used. + if (eid > 1) { + EXPECT_CALL(*data_loader_ptr, UnregisterRequest(eid)) + .WillOnce(Return(eid - 1)); + } + EngineReloadResponse response; + EXPECT_EQ(engine.MaybeReloadEngine(&response), eid == 1); + } + + EXPECT_CALL(*data_manager_ptr, GetDataVersion()) + .WillRepeatedly(Return(data_version)); + + // Finally rollback to the new engine 1. + EXPECT_EQ(engine.GetDataVersion(), data_version); +} + } // namespace engine } // namespace mozc diff --git a/src/session/session_handler_test.cc b/src/session/session_handler_test.cc index 9090a7a03..9097d6c52 100644 --- a/src/session/session_handler_test.cc +++ b/src/session/session_handler_test.cc @@ -613,7 +613,8 @@ TEST_F(SessionHandlerTest, SyncDataTest) { handler.EvalCommand(&command); } -// Tests the interaction with EngineBuilder for successful Engine +// TODO(b/326372188): Reduce redundancy with EngineTest. +// Tests the interaction with DataLoader for successful Engine // reload event. TEST_F(SessionHandlerTest, EngineReloadSuccessfulScenarioTest) { auto data_manager = std::make_unique(); @@ -662,6 +663,7 @@ TEST_F(SessionHandlerTest, EngineReloadSuccessfulScenarioTest) { EXPECT_EQ(handler.engine().GetDataVersion(), data_version); } +// TODO(b/326372188): Reduce redundancy with EngineTest. // Tests situations to handle multiple new requests. TEST_F(SessionHandlerTest, EngineUpdateSuccessfulScenarioTest) { auto data_loader = std::make_unique(); @@ -736,7 +738,8 @@ TEST_F(SessionHandlerTest, EngineUpdateSuccessfulScenarioTest) { EXPECT_EQ(handler.engine().GetDataVersion(), data_version2); } -// Tests the interaction with EngineBuilder in the situation where +// TODO(b/326372188): Reduce redundancy with EngineTest. +// Tests the interaction with DataLoader in the situation where // requested data is broken. TEST_F(SessionHandlerTest, EngineReloadInvalidDataTest) { auto data_loader = std::make_unique(); @@ -767,8 +770,7 @@ TEST_F(SessionHandlerTest, EngineReloadInvalidDataTest) { }))); EXPECT_CALL(*data_loader_ptr, UnregisterRequest(1)).WillOnce(Return(0)); - // Build() is called, but it returns invalid engine, so new engine is not - // used. + // Build() is called, but it returns invalid data, so new data is not used. EXPECT_EQ(&handler.engine(), old_engine_ptr); uint64_t id = 0; ASSERT_TRUE(CreateSession(&handler, &id)); @@ -784,6 +786,7 @@ TEST_F(SessionHandlerTest, EngineReloadInvalidDataTest) { EXPECT_EQ(&handler.engine(), old_engine_ptr); } +// TODO(b/326372188): Reduce redundancy with EngineTest. // Tests the rollback scenario TEST_F(SessionHandlerTest, EngineRollbackDataTest) { auto data_loader = std::make_unique(); @@ -846,7 +849,7 @@ TEST_F(SessionHandlerTest, EngineRollbackDataTest) { EXPECT_EQ(handler.engine().GetDataVersion(), data_version); } -// Tests the interaction with EngineBuilder in the situation where +// Tests the interaction with DataLoader in the situation where // sessions exist in create session event. TEST_F(SessionHandlerTest, EngineReloadSessionExistsTest) { auto data_manager = std::make_unique();