From 8a6197d241e715ef23251cc161150a5e7e7f99b4 Mon Sep 17 00:00:00 2001 From: Alexander Taepper Date: Mon, 14 Oct 2024 16:47:23 +0200 Subject: [PATCH] fix: prevent setDatabase starvation which leads to no updates of currently loaded database --- include/silo_api/database_mutex.h | 20 +++++++------------ src/silo_api/database_directory_watcher.cpp | 9 +++++++-- src/silo_api/database_mutex.cpp | 19 ++++++------------ src/silo_api/info_handler.cpp | 8 ++++---- src/silo_api/query_handler.cpp | 4 ++-- src/silo_api/request_handler_factory.test.cpp | 20 ++++++++----------- 6 files changed, 34 insertions(+), 46 deletions(-) diff --git a/include/silo_api/database_mutex.h b/include/silo_api/database_mutex.h index 6a1bdd8e2..7dd47675b 100644 --- a/include/silo_api/database_mutex.h +++ b/include/silo_api/database_mutex.h @@ -1,20 +1,11 @@ #pragma once -#include +#include #include "silo/database.h" namespace silo_api { -class FixedDatabase { - std::shared_lock lock; - - public: - FixedDatabase(const silo::Database& database, std::shared_lock&& mutex); - - const silo::Database& database; -}; - class UninitializedDatabaseException : public std::runtime_error { public: UninitializedDatabaseException() @@ -22,15 +13,18 @@ class UninitializedDatabaseException : public std::runtime_error { }; class DatabaseMutex { - std::shared_mutex mutex; - silo::Database database; bool is_initialized = false; + std::shared_ptr 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 getDatabase(); }; } // namespace silo_api diff --git a/src/silo_api/database_directory_watcher.cpp b/src/silo_api/database_directory_watcher.cpp index 0c97d11a9..52cfd89f9 100644 --- a/src/silo_api/database_directory_watcher.cpp +++ b/src/silo_api/database_directory_watcher.cpp @@ -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) { @@ -137,6 +137,11 @@ void silo_api::DatabaseDirectoryWatcher::checkDirectoryForData(Poco::Timer& /*ti try { database_mutex.setDatabase(silo::Database::loadDatabaseState(most_recent_database_state->first )); + 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) { @@ -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.", most_recent_database_state->first.string() ); } \ No newline at end of file diff --git a/src/silo_api/database_mutex.cpp b/src/silo_api/database_mutex.cpp index f9ec600c9..2bddad1a3 100644 --- a/src/silo_api/database_mutex.cpp +++ b/src/silo_api/database_mutex.cpp @@ -1,31 +1,24 @@ #include "silo_api/database_mutex.h" -#include +#include #include #include "silo/database.h" namespace silo_api { -silo_api::FixedDatabase::FixedDatabase( - const silo::Database& database, - std::shared_lock&& mutex -) - : lock(std::move(mutex)), - database(database) {} - void silo_api::DatabaseMutex::setDatabase(silo::Database&& new_database) { - const std::unique_lock lock(mutex); - database = std::move(new_database); + auto new_database_pointer = std::make_shared(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_api::DatabaseMutex::getDatabase() { if (!is_initialized) { throw silo_api::UninitializedDatabaseException(); } - std::shared_lock lock(mutex); - return {database, std::move(lock)}; + return std::atomic_load(&database); } } // namespace silo_api diff --git a/src/silo_api/info_handler.cpp b/src/silo_api/info_handler.cpp index b1dff7613..b0be18bb4 100644 --- a/src/silo_api/info_handler.cpp +++ b/src/silo_api/info_handler.cpp @@ -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; diff --git a/src/silo_api/query_handler.cpp b/src/silo_api/query_handler.cpp index 98ae1d36d..852f96368 100644 --- a/src/silo_api/query_handler.cpp +++ b/src/silo_api/query_handler.cpp @@ -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(); diff --git a/src/silo_api/request_handler_factory.test.cpp b/src/silo_api/request_handler_factory.test.cpp index 2940b7ef0..29b439bfc 100644 --- a/src/silo_api/request_handler_factory.test.cpp +++ b/src/silo_api/request_handler_factory.test.cpp @@ -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 mock_database = std::make_shared(); - silo_api::FixedDatabase getDatabase() override { - std::shared_lock lock(mutex); - return {mock_database, std::move(lock)}; - } + std::shared_ptr getDatabase() override { return mock_database; } }; class RequestHandlerTestFixture : public ::testing::Test { @@ -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"); @@ -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"); @@ -144,8 +140,8 @@ TEST_F(RequestHandlerTestFixture, handlesPostQueryRequest) { }; std::vector 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");