Skip to content

Commit

Permalink
Unbox ResponseFuture.
Browse files Browse the repository at this point in the history 
BackgroundFuture is now properly movable.

PiperOrigin-RevId: 621553074
  • Loading branch information
@tomokinat @hiroyuki-komatsu
tomokinat authored and hiroyuki-komatsu committed Apr 3, 2024
1 parent abb647f commit 212b823
Show file tree
Hide file tree
Showing 5 changed files with 168 additions and 196 deletions.
19 changes: 9 additions & 10 deletions src/engine/data_loader.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,8 +182,7 @@ DataLoader::Response BuildResponse(uint64_t id, EngineReloadRequest request) {
}
} // namespace

std::unique_ptr<DataLoader::ResponseFuture> DataLoader::Build(
uint64_t id) const {
DataLoader::ResponseFuture DataLoader::Build(uint64_t id) const {
EngineReloadRequest request;
// Finds the request associated with `id`.
{
Expand All @@ -192,7 +191,7 @@ std::unique_ptr<DataLoader::ResponseFuture> DataLoader::Build(
std::find_if(requests_.begin(), requests_.end(),
[id](const RequestData &v) { return v.id == id; });
if (it == requests_.end()) {
return std::make_unique<DataLoader::ResponseFuture>([id]() {
return DataLoader::ResponseFuture([id]() {
Response response;
response.id = id;
response.response.set_status(EngineReloadResponse::DATA_MISSING);
Expand All @@ -202,16 +201,15 @@ std::unique_ptr<DataLoader::ResponseFuture> DataLoader::Build(
request = it->request;
}

return std::make_unique<DataLoader::ResponseFuture>(
[id, request = std::move(request)]() {
return BuildResponse(id, request);
});
return DataLoader::ResponseFuture([id, request = std::move(request)]() {
return BuildResponse(id, request);
});
}

void DataLoader::MaybeBuildNewData() {
// Checks if an existing build process, or already using the top request.
if (loader_response_future_ || current_request_id_ == top_request_id_ ||
top_request_id_ == 0) {
if (loader_response_future_.has_value() ||
current_request_id_ == top_request_id_ || top_request_id_ == 0) {
return;
}

Expand All @@ -226,7 +224,8 @@ void DataLoader::MaybeBuildNewData() {
}

bool DataLoader::IsBuildResponseReady() const {
return loader_response_future_ && loader_response_future_->Ready();
return loader_response_future_.has_value() &&
loader_response_future_->Ready();
}

std::unique_ptr<DataLoader::Response>
Expand Down
9 changes: 4 additions & 5 deletions src/engine/data_loader.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
#include <atomic>
#include <cstdint>
#include <memory>
#include <optional>
#include <vector>

#include "absl/base/thread_annotations.h"
Expand Down Expand Up @@ -84,10 +85,8 @@ class DataLoader {

// Builds the new engine associated with `id`.
// This method returns the future object immediately.
// Since BackgroundFuture is not movable/copyable, we wrap it with
// std::unique_ptr. This method doesn't return nullptr. All errors
// are stored in EngineReloadResponse::response::status.
virtual std::unique_ptr<ResponseFuture> Build(uint64_t id) const;
// All errors are stored in EngineReloadResponse::response::status.
virtual ResponseFuture Build(uint64_t id) const;

void Clear();

Expand Down Expand Up @@ -130,7 +129,7 @@ class DataLoader {
// 0 means that no data has been updated yet.
std::atomic<uint64_t> current_request_id_ = 0;

std::unique_ptr<DataLoader::ResponseFuture> loader_response_future_;
std::optional<DataLoader::ResponseFuture> loader_response_future_;
// used only in unittest to perform blocking behavior.
bool always_wait_for_loader_response_future_ = false;
};
Expand Down
56 changes: 24 additions & 32 deletions src/engine/data_loader_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,10 @@ TEST_P(DataLoaderTest, BasicTest) {
request_.set_magic_number(kMockMagicNumber);

const uint64_t id = loader_.RegisterRequest(request_);
std::unique_ptr<DataLoader::ResponseFuture> response_future =
loader_.Build(id);
DataLoader::ResponseFuture response_future = loader_.Build(id);

response_future->Wait();
const DataLoader::Response &response = response_future->Get();
response_future.Wait();
const DataLoader::Response &response = response_future.Get();

DataManager data_manager;
data_manager.InitFromFile(mock_data_path_, kMockMagicNumber);
Expand Down Expand Up @@ -125,10 +124,9 @@ TEST_P(DataLoaderTest, BasicTest) {
request_.set_magic_number(kMockMagicNumber);
const uint64_t id = loader_.RegisterRequest(request_);

std::unique_ptr<DataLoader::ResponseFuture> response_future =
loader_.Build(id);
response_future->Wait();
const DataLoader::Response &response = response_future->Get();
DataLoader::ResponseFuture response_future = loader_.Build(id);
response_future.Wait();
const DataLoader::Response &response = response_future.Get();

DataManager data_manager;
data_manager.InitFromFile(src_path, kMockMagicNumber);
Expand Down Expand Up @@ -173,11 +171,10 @@ TEST_P(DataLoaderTest, AsyncBuildRepeatedly) {
}
}

std::unique_ptr<DataLoader::ResponseFuture> response_future =
loader_.Build(latest_id);
DataLoader::ResponseFuture response_future = loader_.Build(latest_id);

response_future->Wait();
const DataLoader::Response &response = response_future->Get();
response_future.Wait();
const DataLoader::Response &response = response_future.Get();

DataManager data_manager;
data_manager.InitFromFile(last_path, kMockMagicNumber);
Expand All @@ -203,11 +200,10 @@ TEST_P(DataLoaderTest, AsyncBuildWithoutInstall) {
request_.set_magic_number(kMockMagicNumber);
const uint64_t id = loader_.RegisterRequest(request_);

std::unique_ptr<DataLoader::ResponseFuture> response_future =
loader_.Build(id);
DataLoader::ResponseFuture response_future = loader_.Build(id);

response_future->Wait();
const DataLoader::Response &response = response_future->Get();
response_future.Wait();
const DataLoader::Response &response = response_future.Get();

DataManager data_manager;
data_manager.InitFromFile(mock_data_path_, kMockMagicNumber);
Expand Down Expand Up @@ -242,11 +238,10 @@ TEST_P(DataLoaderTest, AsyncBuildWithInstall) {
request_.set_magic_number(kMockMagicNumber);
const uint64_t id = loader_.RegisterRequest(request_);

std::unique_ptr<DataLoader::ResponseFuture> response_future =
loader_.Build(id);
DataLoader::ResponseFuture response_future = loader_.Build(id);

response_future->Wait();
const DataLoader::Response &response = response_future->Get();
response_future.Wait();
const DataLoader::Response &response = response_future.Get();

// Builder should be ready now.
EXPECT_EQ(response.response.status(), EngineReloadResponse::RELOAD_READY);
Expand Down Expand Up @@ -278,11 +273,10 @@ TEST_P(DataLoaderTest, FailureCaseDataBroken) {
request_.set_magic_number(kMockMagicNumber);
const uint64_t id = loader_.RegisterRequest(request_);

std::unique_ptr<DataLoader::ResponseFuture> response_future =
loader_.Build(id);
DataLoader::ResponseFuture response_future = loader_.Build(id);

response_future->Wait();
const DataLoader::Response &response = response_future->Get();
response_future.Wait();
const DataLoader::Response &response = response_future.Get();

EXPECT_EQ(response.response.status(), EngineReloadResponse::DATA_BROKEN);
EXPECT_FALSE(response.modules);
Expand All @@ -297,11 +291,10 @@ TEST_P(DataLoaderTest, InvalidId) {
const uint64_t id =
loader_.RegisterRequest(request_) + 1; // + 1 to make invalid id.

std::unique_ptr<DataLoader::ResponseFuture> response_future =
loader_.Build(id);
DataLoader::ResponseFuture response_future = loader_.Build(id);

response_future->Wait();
const DataLoader::Response &response = response_future->Get();
response_future.Wait();
const DataLoader::Response &response = response_future.Get();

EXPECT_EQ(response.response.status(), EngineReloadResponse::DATA_MISSING);
EXPECT_FALSE(response.modules);
Expand All @@ -315,11 +308,10 @@ TEST_P(DataLoaderTest, FailureCaseFileDoesNotExist) {
request_.set_magic_number(kMockMagicNumber);

const uint64_t id = loader_.RegisterRequest(request_);
std::unique_ptr<DataLoader::ResponseFuture> response_future =
loader_.Build(id);
DataLoader::ResponseFuture response_future = loader_.Build(id);

response_future->Wait();
const DataLoader::Response &response = response_future->Get();
response_future.Wait();
const DataLoader::Response &response = response_future.Get();

EXPECT_EQ(response.response.status(), EngineReloadResponse::MMAP_FAILURE);
EXPECT_FALSE(response.modules);
Expand Down
124 changes: 58 additions & 66 deletions src/engine/engine_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,7 @@ class MockSpellchecker : public engine::SpellcheckerInterface {

class MockDataLoader : public DataLoader {
public:
MOCK_METHOD(std::unique_ptr<ResponseFuture>, Build, (uint64_t),
(const override));
MOCK_METHOD(ResponseFuture, Build, (uint64_t), (const override));
};
} // namespace

Expand Down Expand Up @@ -115,16 +114,15 @@ TEST(EngineTest, DataLoadSuccessfulScenarioTest) {
const uint64_t id = data_loader->GetRequestId(request);

EXPECT_CALL(*data_loader, Build(id))
.WillOnce(Return(
std::make_unique<BackgroundFuture<DataLoader::Response>>([&]() {
// takes 0.1 seconds to make engine.
absl::SleepFor(absl::Milliseconds(100));
DataLoader::Response result;
result.id = id;
result.response.set_status(EngineReloadResponse::RELOAD_READY);
result.modules = std::move(modules);
return result;
})));
.WillOnce(Return(BackgroundFuture<DataLoader::Response>([&]() {
// takes 0.1 seconds to make engine.
absl::SleepFor(absl::Milliseconds(100));
DataLoader::Response result;
result.id = id;
result.response.set_status(EngineReloadResponse::RELOAD_READY);
result.modules = std::move(modules);
return result;
})));

absl::StatusOr<std::unique_ptr<Engine>> engine_status =
Engine::CreateMobileEngine(std::make_unique<testing::MockDataManager>());
Expand Down Expand Up @@ -179,15 +177,14 @@ TEST(EngineTest, DataUpdateSuccessfulScenarioTest) {
const uint64_t id2 = data_loader->GetRequestId(request2);

EXPECT_CALL(*data_loader, Build(id1))
.WillOnce(Return(
std::make_unique<BackgroundFuture<DataLoader::Response>>([&]() {
absl::SleepFor(absl::Milliseconds(100));
DataLoader::Response result;
result.id = id1;
result.response.set_status(EngineReloadResponse::RELOAD_READY);
result.modules = std::move(modules1);
return result;
})));
.WillOnce(Return(BackgroundFuture<DataLoader::Response>([&]() {
absl::SleepFor(absl::Milliseconds(100));
DataLoader::Response result;
result.id = id1;
result.response.set_status(EngineReloadResponse::RELOAD_READY);
result.modules = std::move(modules1);
return result;
})));

absl::StatusOr<std::unique_ptr<Engine>> engine_status =
Engine::CreateMobileEngine(std::make_unique<testing::MockDataManager>());
Expand All @@ -206,15 +203,14 @@ TEST(EngineTest, DataUpdateSuccessfulScenarioTest) {
EXPECT_EQ(engine.GetDataVersion(), data_version1);

EXPECT_CALL(*data_loader_ptr, Build(id2))
.WillOnce(Return(
std::make_unique<BackgroundFuture<DataLoader::Response>>([&]() {
absl::SleepFor(absl::Milliseconds(100));
DataLoader::Response result;
result.id = id2;
result.response.set_status(EngineReloadResponse::RELOAD_READY);
result.modules = std::move(modules2);
return result;
})));
.WillOnce(Return(BackgroundFuture<DataLoader::Response>([&]() {
absl::SleepFor(absl::Milliseconds(100));
DataLoader::Response result;
result.id = id2;
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(request2));
Expand Down Expand Up @@ -244,14 +240,13 @@ TEST(EngineTest, ReloadInvalidDataTest) {
EXPECT_TRUE(engine.SendEngineReloadRequest(request));

EXPECT_CALL(*data_loader_ptr, Build(id))
.WillOnce(Return(
std::make_unique<BackgroundFuture<DataLoader::Response>>([&]() {
absl::SleepFor(absl::Milliseconds(100));
DataLoader::Response result;
result.id = id;
result.response.set_status(EngineReloadResponse::DATA_BROKEN);
return result;
})));
.WillOnce(Return(BackgroundFuture<DataLoader::Response>([&]() {
absl::SleepFor(absl::Milliseconds(100));
DataLoader::Response result;
result.id = id;
result.response.set_status(EngineReloadResponse::DATA_BROKEN);
return result;
})));

// Build() is called, but it returns invalid data, so new data is not used.
EngineReloadResponse response;
Expand Down Expand Up @@ -306,36 +301,33 @@ TEST(EngineTest, RollbackDataTest) {
ASSERT_TRUE(engine.SendEngineReloadRequest(request3_broken));

EXPECT_CALL(*data_loader_ptr, Build(id3))
.WillOnce(Return(
std::make_unique<BackgroundFuture<DataLoader::Response>>([&]() {
absl::SleepFor(absl::Milliseconds(100));
DataLoader::Response result;
result.id = id3;
result.response.set_status(EngineReloadResponse::DATA_BROKEN);
*result.response.mutable_request() = request3_broken;
return result;
})));
.WillOnce(Return(BackgroundFuture<DataLoader::Response>([&]() {
absl::SleepFor(absl::Milliseconds(100));
DataLoader::Response result;
result.id = id3;
result.response.set_status(EngineReloadResponse::DATA_BROKEN);
*result.response.mutable_request() = request3_broken;
return result;
})));
EXPECT_CALL(*data_loader_ptr, Build(id2))
.WillOnce(Return(
std::make_unique<BackgroundFuture<DataLoader::Response>>([&]() {
absl::SleepFor(absl::Milliseconds(100));
DataLoader::Response result;
result.id = id2;
result.response.set_status(EngineReloadResponse::DATA_BROKEN);
*result.response.mutable_request() = request2_broken;
return result;
})));
.WillOnce(Return(BackgroundFuture<DataLoader::Response>([&]() {
absl::SleepFor(absl::Milliseconds(100));
DataLoader::Response result;
result.id = id2;
result.response.set_status(EngineReloadResponse::DATA_BROKEN);
*result.response.mutable_request() = request2_broken;
return result;
})));
EXPECT_CALL(*data_loader_ptr, Build(id1))
.WillOnce(Return(
std::make_unique<BackgroundFuture<DataLoader::Response>>([&]() {
absl::SleepFor(absl::Milliseconds(100));
DataLoader::Response result;
result.id = id1;
result.response.set_status(EngineReloadResponse::RELOAD_READY);
*result.response.mutable_request() = request1_ready;
result.modules = std::move(modules);
return result;
})));
.WillOnce(Return(BackgroundFuture<DataLoader::Response>([&]() {
absl::SleepFor(absl::Milliseconds(100));
DataLoader::Response result;
result.id = id1;
result.response.set_status(EngineReloadResponse::RELOAD_READY);
*result.response.mutable_request() = request1_ready;
result.modules = std::move(modules);
return result;
})));

// Both request#3 and request#2 are broken. request#1 is immediately used as a
// fallback.
Expand Down
Loading

0 comments on commit 212b823

Please sign in to comment.