Merge branch 'google:master' into master

src/base/file_util.cc

@@ -54,12 +54,12 @@
 #include "base/singleton.h"
 
 #ifdef _WIN32
-#include "base/util.h"
-#include "base/win32/wide_char.h"
-
 #include <wil/resource.h>
 #include <windows.h>
 
+#include "base/util.h"
+#include "base/win32/wide_char.h"
+
 #else  // _WIN32
 #include <sys/stat.h>
 #include <unistd.h>
@@ -198,6 +198,10 @@ absl::Status FileUtil::CreateDirectory(const std::string &path) {
 }
 
 absl::Status FileUtilImpl::CreateDirectory(const std::string &path) const {
+  // If the path already exists, returns OkStatus and does nothing.
+  if (const absl::Status status = DirectoryExists(path); status.ok()) {
+    return absl::OkStatus();
+  }
 #if defined(_WIN32)
   const std::wstring wide = win32::Utf8ToWide(path);
   if (wide.empty()) {

src/base/file_util.h

@@ -92,6 +92,7 @@ class FileUtil {
   ~FileUtil() = delete;
 
   // Creates a directory. Does not create directories in the way to the path.
+  // If the directory already exists, returns OkStatus and does nothing.
   static absl::Status CreateDirectory(const std::string &path);
 
   // Removes an empty directory.

src/base/file_util_test.cc

@@ -74,6 +74,9 @@ TEST(FileUtilTest, CreateDirectory) {
   EXPECT_OK(FileUtil::CreateDirectory(dirpath));
   EXPECT_OK(FileUtil::DirectoryExists(dirpath));
 
+  // Create the same directory again.
+  EXPECT_OK(FileUtil::CreateDirectory(dirpath));
+
   // Delete the directory.
   ASSERT_OK(FileUtil::RemoveDirectory(dirpath));
   ASSERT_FALSE(FileUtil::FileExists(dirpath).ok());

src/engine/BUILD.bazel

@@ -61,6 +61,7 @@ mozc_cc_test(
     srcs = ["data_loader_test.cc"],
     data = [
         "data_loader_test.cc",
+        "//data_manager/oss:mozc.data",
         "//data_manager/testing:mock_mozc.data",
     ],
     deps = [

src/engine/README.md

@@ -0,0 +1,57 @@
+# engine/
+
+`engine/` is the directory for components managing objects:
+
+*   loaded from the LM data
+*   used by converters like Converter, Predictor, Rewriter, etc.
+
+## Engine
+
+`Engine` takes responsibility for:
+
+*   interface to provide a `Converter` object (mainly for `Session`)
+*   ownership of the `Modules` object and objects created from the `Modules`
+    object
+*   loading a LM from a data file
+
+## Modules
+
+`Modules` takes ownership of objects initialized from `DataManager`. When a new
+LM is loaded, a new `Modules` object is created as a new `DataManager` object is
+created.
+
+## DataLoader
+
+`DataLoader` loads a LM data file and create a `Modules` object. `DataLoader`
+receives requests for LM data loadings and manages the priority among the
+multiple requests.
+
+### Priority of data loading
+
+Each data loading request has a priority based on the request type (e.g. 10, 40,
+100). The lower value has the higher priority. The new LM is loaded only when
+the priority of the new data is higher the current LM.
+
+`DataLoader` always processes the highest priority request. The caller can get
+the highest priority LM from the `DataLoader`. (Note, it is not true yet for the
+current implementation. this is one of the goal of the refactoring).
+
+### Timing of data loading
+
+When `DataLoader` receives a loading request, `DataLoader` will:
+
+*   Do nothing, if the priority of the new request is lower than the priority of
+    the current LM. In this case, the following items are not applicable.
+*   Wait for the current loading task, if `DataLodear` is loading another LM
+    now.
+*   Start a new loading task for the new request, if the new request has the
+    higher priority than the loaded LM.
+
+### Concurrency policy of DataLoader
+
+While `DataLoader` uses a sub-thread to load a LM data file, it is designed to
+be thread-compatible similarly to std::vector and other Mozc objects. It means
+that callers can adopt the same concurrency policy with other objects (you don't
+need extra care about this object). It should be fine while you use it in a
+single thread, and you should have some treatments as well as other components
+if you use it in multiple threads.

src/engine/data_loader.cc

@@ -204,11 +204,24 @@ bool DataLoader::StartNewDataBuildTask() {
     return false;
   }
 
-  // Checks if the currently loading process is the highest priority request.
-  // Note, Get() may wait until loader_response_future_ is ready.
-  if (loader_response_future_.has_value() &&
-      loader_response_future_->Get().id == top_request_id_) {
-    return true;
+  if (loader_response_future_.has_value()) {
+    // Checks if the currently loading task is the highest priority request.
+    // Note, Get() may wait until loader_response_future_ is ready.
+    if (loader_response_future_->Get().id == top_request_id_) {
+      return true;
+    }
+
+    // Do not use this result as the new request is already queued.
+    mozc::DataLoader::Response loader_response =
+        (*std::move(loader_response_future_)).Get();
+    loader_response_future_.reset();
+
+    // If the response is invalid, report it as failure not to retry for the
+    // same request in future.
+    if (!loader_response.modules || loader_response.response.status() !=
+                                        EngineReloadResponse::RELOAD_READY) {
+      ReportLoadFailure(loader_response.id);
+    }
   }
 
   LOG(INFO) << "Building a new module (current ID =" << current_request_id_
@@ -240,7 +253,7 @@ DataLoader::MaybeMoveDataLoaderResponse() {
 
   // Returns the new DataLoader::Response that is ready to use.
   mozc::DataLoader::Response loader_response =
-      std::move(*loader_response_future_).Get();
+      (*std::move(loader_response_future_)).Get();
   loader_response_future_.reset();
 
   return std::make_unique<DataLoader::Response>(std::move(loader_response));

src/engine/data_loader_test.cc

@@ -50,6 +50,7 @@ namespace mozc {
 namespace {
 
 constexpr absl::string_view kMockMagicNumber = "MOCK";
+constexpr absl::string_view kOssMagicNumber = "\xEFMOZC\x0D\x0A";
 
 struct Param {
   EngineReloadRequest::EngineType type;
@@ -64,6 +65,27 @@ class DataLoaderTest : public testing::TestWithTempUserProfile,
             testing::GetSourcePath({MOZC_SRC_COMPONENTS("data_manager"),
                                     "testing", "mock_mozc.data"})) {
     LOG(INFO) << mock_data_path_;
+
+    const std::string mock_path = testing::GetSourcePath(
+        {MOZC_SRC_COMPONENTS("data_manager"), "testing", "mock_mozc.data"});
+    mock_request_.set_engine_type(EngineReloadRequest::MOBILE);
+    mock_request_.set_file_path(mock_path);
+    mock_request_.set_magic_number(kMockMagicNumber);
+    mock_request_.set_priority(50);
+
+    const std::string oss_path = testing::GetSourcePath(
+        {MOZC_SRC_COMPONENTS("data_manager"), "oss", "mozc.data"});
+    oss_request_.set_engine_type(EngineReloadRequest::MOBILE);
+    oss_request_.set_file_path(oss_path);
+    oss_request_.set_magic_number(kOssMagicNumber);
+    oss_request_.set_priority(50);
+
+    const std::string invalid_path = testing::GetSourcePath(
+        {MOZC_SRC_COMPONENTS("data_manager"), "invalid", "mozc.data"});
+    invalid_path_request_.set_engine_type(EngineReloadRequest::MOBILE);
+    invalid_path_request_.set_file_path(invalid_path);
+    invalid_path_request_.set_magic_number(kOssMagicNumber);
+    invalid_path_request_.set_priority(50);
   }
 
   void Clear() {
@@ -74,6 +96,9 @@ class DataLoaderTest : public testing::TestWithTempUserProfile,
   const std::string mock_data_path_;
   DataLoader loader_;
   EngineReloadRequest request_;
+  EngineReloadRequest mock_request_;
+  EngineReloadRequest oss_request_;
+  EngineReloadRequest invalid_path_request_;
 };
 
 TEST_P(DataLoaderTest, BasicTest) {
@@ -370,6 +395,50 @@ TEST_P(DataLoaderTest, RegisterRequestTest) {
   EXPECT_EQ(0, unregister_request("buzz", kPLow));
 }
 
+TEST_P(DataLoaderTest, LowPriorityReuqestTest) {
+  EngineReloadRequest low_priority_request = mock_request_;
+  low_priority_request.set_priority(100);
+  ASSERT_GT(low_priority_request.priority(), oss_request_.priority());
+
+  // Starts a new build of a higher request at first.
+  loader_.RegisterRequest(oss_request_);
+  EXPECT_TRUE(loader_.StartNewDataBuildTask());
+  // It is usually not ready yet, although it depends on the task volume.
+  EXPECT_FALSE(loader_.IsBuildResponseReady());
+
+  // Tries another build of a lower request. It waits for the previous task.
+  // The new task is not started because of the priority.
+  loader_.RegisterRequest(low_priority_request);
+  EXPECT_TRUE(loader_.StartNewDataBuildTask());
+
+  // The task of the first request with higher priority should ready.
+  EXPECT_TRUE(loader_.IsBuildResponseReady());
+
+  // The response is built with the first request.
+  std::unique_ptr<DataLoader::Response> response =
+      loader_.MaybeMoveDataLoaderResponse();
+  EXPECT_EQ(response->response.request().file_path(), oss_request_.file_path());
+}
+
+TEST_P(DataLoaderTest, DuprecatedInvalidReuqestTest) {
+  // Starts a new build, but the request is invalid and it will fail.
+  const uint64_t invalid_request_id =
+      loader_.RegisterRequest(invalid_path_request_);
+  EXPECT_TRUE(loader_.StartNewDataBuildTask());
+  EXPECT_FALSE(loader_.IsBuildResponseReady());
+
+  // Sends another and valid request. It waits for the previous task and
+  // records the previous request as invalid.
+  const uint64_t mock_request_id = loader_.RegisterRequest(mock_request_);
+  EXPECT_TRUE(loader_.StartNewDataBuildTask());
+
+  // Registers the first invalid request again, but it is already excluded.
+  const uint64_t top_request_id =
+      loader_.RegisterRequest(invalid_path_request_);
+  EXPECT_EQ(top_request_id, mock_request_id);
+  EXPECT_NE(top_request_id, invalid_request_id);
+}
+
 INSTANTIATE_TEST_SUITE_P(
     DataLoaderTest, DataLoaderTest,
     ::testing::Values(Param{EngineReloadRequest::DESKTOP, "DefaultPredictor"},

src/mac/tweak_installer_files.py

@@ -124,6 +124,12 @@ def TweakQtApps(top_dir: str, oss: bool) -> None:
     if not any(os.scandir(framework_dir)):
       os.rmdir(framework_dir)
 
+    # Codesign again. '-' means psuedo identity.
+    # https://github.com/bazelbuild/rules_apple/blob/3.5.1/apple/internal/codesigning_support.bzl#L42
+    # https://developer.apple.com/documentation/security/seccodesignatureflags/1397793-adhoc
+    codesign = ['/usr/bin/codesign', '--force', '--sign', '-', app_dir]
+    util.RunOrDie(codesign)
+
   main_qt_apps = [
       'ConfigDialog.app',
       'DictionaryTool.app',

src/session/session_handler_test.cc

@@ -664,7 +664,7 @@ TEST_F(SessionHandlerTest, EngineReloadSuccessfulScenarioTest) {
   EXPECT_NE(command.output().id(), 0);
 
   // When the engine is created first, we wait until the engine gets ready.
-  EXPECT_EQ(handler_->engine().GetDataVersion(), mock_version_);
+  EXPECT_EQ(handler_->GetDataVersion(), mock_version_);
 
   // New session is created, but Build is not called as the id is the same.
   ASSERT_EQ(SendMockEngineReloadRequest(*handler_, mock_request_),
@@ -673,7 +673,7 @@ TEST_F(SessionHandlerTest, EngineReloadSuccessfulScenarioTest) {
   uint64_t id = 0;
   ASSERT_TRUE(DeleteSession(*handler_, id));
   ASSERT_TRUE(CreateSession(*handler_, &id));
-  EXPECT_EQ(handler_->engine().GetDataVersion(), mock_version_);
+  EXPECT_EQ(handler_->GetDataVersion(), mock_version_);
 }
 
 // Tests situations to handle multiple new requests.
@@ -685,27 +685,27 @@ TEST_F(SessionHandlerTest, EngineUpdateSuccessfulScenarioTest) {
   // build request is called one per new engine reload request.
   uint64_t id = 0;
   ASSERT_TRUE(CreateSession(*handler_, &id));
-  EXPECT_EQ(handler_->engine().GetDataVersion(), mock_version_);
+  EXPECT_EQ(handler_->GetDataVersion(), mock_version_);
 
   // engine_id = 2
   ASSERT_EQ(SendMockEngineReloadRequest(*handler_, oss_request_),
             EngineReloadResponse::ACCEPTED);
 
   ASSERT_TRUE(DeleteSession(*handler_, id));
   ASSERT_TRUE(CreateSession(*handler_, &id));
-  EXPECT_EQ(handler_->engine().GetDataVersion(), oss_version_);
+  EXPECT_EQ(handler_->GetDataVersion(), oss_version_);
 }
 
 // Tests the interaction with DataLoader in the situation where requested data
 // is broken.
 TEST_F(SessionHandlerTest, EngineReloadInvalidDataTest) {
-  const absl::string_view initial_version = handler_->engine().GetDataVersion();
+  const absl::string_view initial_version = handler_->GetDataVersion();
 
   ASSERT_EQ(SendMockEngineReloadRequest(*handler_, invalid_path_request_),
             EngineReloadResponse::ACCEPTED);
 
   // Build() is called, but it returns invalid data, so new data is not used.
-  EXPECT_EQ(handler_->engine().GetDataVersion(), initial_version);
+  EXPECT_EQ(handler_->GetDataVersion(), initial_version);
 
   // CreateSession does not contain engine_reload_response.
   commands::Command command;
@@ -715,7 +715,7 @@ TEST_F(SessionHandlerTest, EngineReloadInvalidDataTest) {
   EXPECT_FALSE(command.output().has_engine_reload_response());
   EXPECT_NE(command.output().id(), 0);
 
-  EXPECT_EQ(handler_->engine().GetDataVersion(), initial_version);
+  EXPECT_EQ(handler_->GetDataVersion(), initial_version);
 
   // Sends the same request again, but the request is already marked as
   // unregistered.
@@ -724,7 +724,7 @@ TEST_F(SessionHandlerTest, EngineReloadInvalidDataTest) {
             EngineReloadResponse::ACCEPTED);
   ASSERT_TRUE(DeleteSession(*handler_, id));
   ASSERT_TRUE(CreateSession(*handler_, &id));
-  EXPECT_EQ(handler_->engine().GetDataVersion(), initial_version);
+  EXPECT_EQ(handler_->GetDataVersion(), initial_version);
 }
 
 // Tests the rollback scenario
@@ -746,7 +746,7 @@ TEST_F(SessionHandlerTest, EngineRollbackDataTest) {
   }
 
   // Finally rollback to the new engine with the first request.
-  EXPECT_EQ(handler_->engine().GetDataVersion(), mock_version_);
+  EXPECT_EQ(handler_->GetDataVersion(), mock_version_);
 }
 
 // Tests the interaction with DataLoader in the situation where
@@ -759,7 +759,7 @@ TEST_F(SessionHandlerTest, EngineReloadSessionExistsTest) {
   // As a session is created before data is loaded, engine is not reloaded yet.
   uint64_t id1 = 0;
   ASSERT_TRUE(CreateSession(*handler_, &id1));
-  EXPECT_EQ(handler_->engine().GetDataVersion(), initial_version);
+  EXPECT_EQ(handler_->GetDataVersion(), initial_version);
   EXPECT_EQ(&handler_->engine(), old_engine_ptr);
 
   ASSERT_EQ(SendMockEngineReloadRequest(*handler_, mock_request_),
@@ -770,8 +770,8 @@ TEST_F(SessionHandlerTest, EngineReloadSessionExistsTest) {
   uint64_t id2 = 0;
   ASSERT_TRUE(CreateSession(*handler_, &id2));
   EXPECT_EQ(&handler_->engine(), old_engine_ptr);
-  EXPECT_EQ(handler_->engine().GetDataVersion(), initial_version);
-  EXPECT_NE(handler_->engine().GetDataVersion(), mock_version_);
+  EXPECT_EQ(handler_->GetDataVersion(), initial_version);
+  EXPECT_NE(handler_->GetDataVersion(), mock_version_);
 
   // All the sessions were deleted.
   ASSERT_TRUE(DeleteSession(*handler_, id1));
@@ -783,7 +783,7 @@ TEST_F(SessionHandlerTest, EngineReloadSessionExistsTest) {
   ASSERT_TRUE(CreateSession(*handler_, &id3));
   // New data is reloaded, but the engine is the same object.
   EXPECT_EQ(&handler_->engine(), old_engine_ptr);
-  EXPECT_EQ(handler_->engine().GetDataVersion(), mock_version_);
+  EXPECT_EQ(handler_->GetDataVersion(), mock_version_);
 }
 
 TEST_F(SessionHandlerTest, GetServerVersionTest) {
@@ -818,7 +818,7 @@ TEST_F(SessionHandlerTest, ReloadFromMinimalEngine) {
   uint64_t id = 0;
   ASSERT_TRUE(CreateSession(handler, &id));
   EXPECT_EQ(handler.engine().GetPredictorName(), "MobilePredictor");
-  EXPECT_EQ(handler.engine().GetDataVersion(), mock_version_);
+  EXPECT_EQ(handler.GetDataVersion(), mock_version_);
 }