Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: prevent setDatabase starvation which leads to no updates of currently loaded database #613

Merged
merged 1 commit into from
Oct 15, 2024
Merged

fix: prevent setDatabase starvation which leads to no updates of currently loaded database #613

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 7 additions & 13 deletions include/silo_api/database_mutex.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,36 +1,30 @@
#pragma once

#include <shared_mutex>
#include <memory>

#include "silo/database.h"

namespace silo_api {

class FixedDatabase {
std::shared_lock<std::shared_mutex> lock;

public:
FixedDatabase(const silo::Database& database, std::shared_lock<std::shared_mutex>&& mutex);

const silo::Database& database;
};

class UninitializedDatabaseException : public std::runtime_error {
public:
UninitializedDatabaseException()
: std::runtime_error("Database not initialized yet") {}
};

class DatabaseMutex {
std::shared_mutex mutex;
silo::Database database;
bool is_initialized = false;
std::shared_ptr<silo::Database> database;

public:
DatabaseMutex() = default;
DatabaseMutex(const DatabaseMutex& other) = delete;
DatabaseMutex(DatabaseMutex&& other) = delete;
DatabaseMutex& operator=(const DatabaseMutex& other) = delete;
DatabaseMutex& operator=(DatabaseMutex&& other) = delete;

void setDatabase(silo::Database&& new_database);

virtual FixedDatabase getDatabase();
virtual std::shared_ptr<silo::Database> getDatabase();
};
} // namespace silo_api
9 changes: 7 additions & 2 deletions src/silo_api/database_directory_watcher.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ void silo_api::DatabaseDirectoryWatcher::checkDirectoryForData(Poco::Timer& /*ti
{
try {
const auto current_data_version_timestamp =
database_mutex.getDatabase().database.getDataVersionTimestamp();
database_mutex.getDatabase()->getDataVersionTimestamp();
const auto most_recent_data_version_timestamp_found =
most_recent_database_state->second.getTimestamp();
if (current_data_version_timestamp >= most_recent_data_version_timestamp_found) {
Expand All @@ -137,6 +137,11 @@ void silo_api::DatabaseDirectoryWatcher::checkDirectoryForData(Poco::Timer& /*ti
try {
database_mutex.setDatabase(silo::Database::loadDatabaseState(most_recent_database_state->first
corneliusroemer marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a timeout on this? When the incident happened, is this where it got stuck indefinitely? Just to help me understand what happened.

));
SPDLOG_INFO(
"New database with version {} successfully loaded.",
most_recent_database_state->first.string()
);
return;
} catch (const std::exception& ex) {
SPDLOG_ERROR(ex.what());
} catch (const std::string& ex) {
Expand All @@ -150,7 +155,7 @@ void silo_api::DatabaseDirectoryWatcher::checkDirectoryForData(Poco::Timer& /*ti
}
}
SPDLOG_INFO(
"New database with version {} successfully loaded.",
"Did not load new database with version {} successfully.",
corneliusroemer marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines 157 to +158
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have log statements in every catch. Should we maybe simply add a clarifying "failed to load new database with version xyz" to the SPDLOG_ERROR(ex.what());?

most_recent_database_state->first.string()
);
}
19 changes: 6 additions & 13 deletions src/silo_api/database_mutex.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,31 +1,24 @@
#include "silo_api/database_mutex.h"

#include <mutex>
#include <atomic>
#include <utility>

#include "silo/database.h"

namespace silo_api {

silo_api::FixedDatabase::FixedDatabase(
const silo::Database& database,
std::shared_lock<std::shared_mutex>&& mutex
)
: lock(std::move(mutex)),
database(database) {}

void silo_api::DatabaseMutex::setDatabase(silo::Database&& new_database) {
const std::unique_lock lock(mutex);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the line where it got stuck before. A log message before would have helped a lot: "Acquiring unique lock to switch out datasets"

database = std::move(new_database);
auto new_database_pointer = std::make_shared<silo::Database>(std::move(new_database));

std::atomic_store(&database, new_database_pointer);
is_initialized = true;
}

silo_api::FixedDatabase silo_api::DatabaseMutex::getDatabase() {
std::shared_ptr<silo::Database> silo_api::DatabaseMutex::getDatabase() {
if (!is_initialized) {
throw silo_api::UninitializedDatabaseException();
}
std::shared_lock<std::shared_mutex> lock(mutex);
return {database, std::move(lock)};
return std::atomic_load(&database);
}

} // namespace silo_api
8 changes: 4 additions & 4 deletions src/silo_api/info_handler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,13 +99,13 @@ void InfoHandler::get(

const auto fixed_database = database.getDatabase();

response.set("data-version", fixed_database.database.getDataVersionTimestamp().value);
response.set("data-version", fixed_database->getDataVersionTimestamp().value);

const bool return_detailed_info = request_parameter.find("details") != request_parameter.end() &&
request_parameter.at("details") == "true";
const nlohmann::json database_info =
return_detailed_info ? nlohmann::json(database.getDatabase().database.detailedDatabaseInfo())
: nlohmann::json(database.getDatabase().database.getDatabaseInfo());
const nlohmann::json database_info = return_detailed_info
? nlohmann::json(fixed_database->detailedDatabaseInfo())
: nlohmann::json(fixed_database->getDatabaseInfo());
response.setContentType("application/json");
std::ostream& out_stream = response.send();
out_stream << database_info;
Expand Down
4 changes: 2 additions & 2 deletions src/silo_api/query_handler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,9 @@ void QueryHandler::post(
try {
const auto fixed_database = database_mutex.getDatabase();

auto query_result = fixed_database.database.executeQuery(query);
auto query_result = fixed_database->executeQuery(query);

response.set("data-version", fixed_database.database.getDataVersionTimestamp().value);
response.set("data-version", fixed_database->getDataVersionTimestamp().value);

response.setContentType("application/x-ndjson");
std::ostream& out_stream = response.send();
Expand Down
20 changes: 8 additions & 12 deletions src/silo_api/request_handler_factory.test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,9 @@ class MockDatabase : public silo::Database {

class MockDatabaseMutex : public silo_api::DatabaseMutex {
public:
std::shared_mutex mutex;
MockDatabase mock_database;
std::shared_ptr<MockDatabase> mock_database = std::make_shared<MockDatabase>();

silo_api::FixedDatabase getDatabase() override {
std::shared_lock<std::shared_mutex> lock(mutex);
return {mock_database, std::move(lock)};
}
std::shared_ptr<silo::Database> getDatabase() override { return mock_database; }
};

class RequestHandlerTestFixture : public ::testing::Test {
Expand Down Expand Up @@ -68,11 +64,11 @@ const int FOUR_MINUTES_IN_SECONDS = 240;
} // namespace

TEST_F(RequestHandlerTestFixture, handlesGetInfoRequest) {
EXPECT_CALL(database_mutex.mock_database, getDatabaseInfo)
EXPECT_CALL(*database_mutex.mock_database, getDatabaseInfo)
.WillRepeatedly(testing::Return(
silo::DatabaseInfo{.sequence_count = 1, .total_size = 2, .n_bitmaps_size = 3}
));
EXPECT_CALL(database_mutex.mock_database, getDataVersionTimestamp)
EXPECT_CALL(*database_mutex.mock_database, getDataVersionTimestamp)
.WillRepeatedly(testing::Return(silo::DataVersion::Timestamp::fromString("1234").value()));

request.setURI("/info");
Expand Down Expand Up @@ -103,9 +99,9 @@ TEST_F(RequestHandlerTestFixture, handlesGetInfoRequestDetails) {

const silo::DetailedDatabaseInfo detailed_database_info = {{{"main", stats}}};

EXPECT_CALL(database_mutex.mock_database, detailedDatabaseInfo)
EXPECT_CALL(*database_mutex.mock_database, detailedDatabaseInfo)
.WillRepeatedly(testing::Return(detailed_database_info));
EXPECT_CALL(database_mutex.mock_database, getDataVersionTimestamp)
EXPECT_CALL(*database_mutex.mock_database, getDataVersionTimestamp)
.WillRepeatedly(testing::Return(silo::DataVersion::Timestamp::fromString("1234").value()));

request.setURI("/info?details=true");
Expand Down Expand Up @@ -144,8 +140,8 @@ TEST_F(RequestHandlerTestFixture, handlesPostQueryRequest) {
};
std::vector<silo::query_engine::QueryResultEntry> tmp{{fields1}, {fields2}};
auto query_result = silo::query_engine::QueryResult::fromVector(std::move(tmp));
EXPECT_CALL(database_mutex.mock_database, executeQuery).WillOnce(testing::Return(query_result));
EXPECT_CALL(database_mutex.mock_database, getDataVersionTimestamp)
EXPECT_CALL(*database_mutex.mock_database, executeQuery).WillOnce(testing::Return(query_result));
EXPECT_CALL(*database_mutex.mock_database, getDataVersionTimestamp)
.WillOnce(testing::Return(silo::DataVersion::Timestamp::fromString("1234").value()));

request.setMethod("POST");
Expand Down
Loading