From 306f486291392351302fdbd6b02ad6842a5187a9 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 15 Jun 2026 14:11:37 +0000 Subject: [PATCH 1/6] Make Database thread-safe via serialized connection access (#9) The library funnels all access through a single shared sqlite3* connection behind a process-wide singleton, with no synchronization. Concurrent use raced: each operation's BEGIN/COMMIT interleaved on the shared connection ("cannot start a transaction within a transaction"), the singleton lifecycle was unguarded, and query error paths closed the shared connection out from under other callers. This serializes access (approach chosen for #9): - Open the connection in serialized mode (sqlite3_open_v2 + SQLITE_OPEN_FULLMUTEX). - Guard every operation that touches db_ with a per-instance mutex, so the per-operation transaction is never interleaved. The lock in the auto-increment path spans the insert and the last_insert_rowid read, so the returned id always corresponds to this insert. - Guard the singleton lifecycle (Initialize/Finalize/Instance) with a static mutex, and make Instance() throw a clear error instead of dereferencing null when the database is not initialized. - Stop closing the shared connection from FetchRecordsQuery/FetchMaxIdQuery error paths (this left a dangling handle and could double-close on Finalize). Adds concurrency tests that hammer the shared instance from multiple threads and assert all operations succeed, every row persists, and ids stay unique. --- include/database.h | 16 ++++- src/database.cc | 27 +++++++- src/queries.cc | 2 - tests/CMakeLists.txt | 5 +- tests/concurrency_test.cc | 141 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 184 insertions(+), 7 deletions(-) create mode 100644 tests/concurrency_test.cc diff --git a/include/database.h b/include/database.h index 3356f59..bde0faa 100644 --- a/include/database.h +++ b/include/database.h @@ -23,6 +23,7 @@ #pragma once #include +#include #include #include #include @@ -98,9 +99,7 @@ class REFLECTION_EXPORT Database { int64_t GetMaxId() const { const auto type_id = typeid(T).name(); const auto& record = GetRecord(type_id); - FetchMaxIdQuery query(db_, record); - const auto max_id = query.GetMaxId(); - return max_id; + return GetMaxId(record); } /// Saves a given record in the database. @@ -208,6 +207,9 @@ class REFLECTION_EXPORT Database { /// Returns a record type from its type information, retrieved from typeid(...).name() static const Reflection& GetRecord(const std::string& type_id); + /// Returns the max id currently stored for a given record (SELECT MAX(id) FROM table) + int64_t GetMaxId(const Reflection& record) const; + /// Creates concrete record types with initialized members, /// based on the textual representation of results from a fetch query template @@ -235,6 +237,14 @@ class REFLECTION_EXPORT Database { void Delete(const Reflection& record, const QueryPredicateBase* predicate) const; static Database* instance_; + + /// Guards the singleton lifecycle (Initialize / Finalize / Instance) + static std::mutex instance_mutex_; + sqlite3* db_; + + /// Serializes all access to the shared connection db_, so that the per-operation + /// BEGIN/COMMIT transaction is never interleaved across threads + mutable std::mutex db_mutex_; }; } // namespace sqlite_reflection diff --git a/src/database.cc b/src/database.cc index 913f50d..0f0f102 100644 --- a/src/database.cc +++ b/src/database.cc @@ -22,6 +22,7 @@ #include "database.h" +#include #include #include "internal/sqlite3.h" @@ -29,12 +30,14 @@ namespace sqlite_reflection { Database* Database::instance_ = nullptr; +std::mutex Database::instance_mutex_; const ReflectionRegister& GetReflectionRegister() { return *GetReflectionRegisterInstance(); } void Database::Initialize(const std::string& path) { + std::lock_guard lock(instance_mutex_); if (instance_ != nullptr) { throw std::invalid_argument("Database has already been initialized"); } @@ -44,6 +47,7 @@ void Database::Initialize(const std::string& path) { } void Database::Finalize() { + std::lock_guard lock(instance_mutex_); if (instance_ != nullptr) { sqlite3_close(instance_->db_); delete instance_; @@ -52,7 +56,10 @@ void Database::Finalize() { } Database::Database(const char* path) : db_(nullptr) { - if (sqlite3_open(path, &db_)) { + // Open in serialized threading mode so the shared connection is safe to use from + // multiple threads; access is additionally serialized through db_mutex_. + const int flags = SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE | SQLITE_OPEN_FULLMUTEX; + if (sqlite3_open_v2(path, &db_, flags, nullptr)) { throw std::invalid_argument("Database could not be initialized"); } @@ -65,10 +72,15 @@ Database::Database(const char* path) : db_(nullptr) { } const Database& Database::Instance() { + std::lock_guard lock(instance_mutex_); + if (instance_ == nullptr) { + throw std::runtime_error("Database has not been initialized; call Database::Initialize() first"); + } return *instance_; } FetchQueryResults Database::Fetch(const Reflection& record, const QueryPredicateBase* predicate) const { + std::lock_guard lock(db_mutex_); FetchRecordsQuery query(db_, record, predicate); return query.GetResults(); } @@ -77,28 +89,41 @@ const Reflection& Database::GetRecord(const std::string& type_id) { return GetReflectionRegister().records.at(type_id); } +int64_t Database::GetMaxId(const Reflection& record) const { + std::lock_guard lock(db_mutex_); + FetchMaxIdQuery query(db_, record); + return query.GetMaxId(); +} + void Database::Save(void* p, const Reflection& record) const { + std::lock_guard lock(db_mutex_); InsertQuery query(db_, record, p); query.Execute(); } int64_t Database::SaveAutoIncrement(void* p, const Reflection& record) const { + // The lock spans both the insert and the rowid read so that, on the shared connection, + // sqlite3_last_insert_rowid() reflects this insert and not one from another thread. + std::lock_guard lock(db_mutex_); InsertQuery query(db_, record, p, true); query.Execute(); return sqlite3_last_insert_rowid(db_); } void Database::Update(void* p, const Reflection& record) const { + std::lock_guard lock(db_mutex_); UpdateQuery query(db_, record, p); query.Execute(); } void Database::Delete(const Reflection& record, const QueryPredicateBase* predicate) const { + std::lock_guard lock(db_mutex_); DeleteQuery query(db_, record, predicate); query.Execute(); } void Database::UnsafeSql(const std::string& raw_sql_query) const { + std::lock_guard lock(db_mutex_); SqlQuery sql(db_, raw_sql_query); sql.Execute(); } diff --git a/src/queries.cc b/src/queries.cc index 092de2b..733cb4a 100644 --- a/src/queries.cc +++ b/src/queries.cc @@ -293,7 +293,6 @@ int64_t FetchMaxIdQuery::GetMaxId() { const auto sql = PrepareSql(); if (sqlite3_prepare_v2(db_, sql.data(), -1, &stmt_, nullptr)) { - sqlite3_close(db_); throw std::runtime_error("Could not retrieve max id for table " + record_.name); } @@ -323,7 +322,6 @@ FetchQueryResults FetchRecordsQuery::GetResults() { const auto sql = PrepareSql(); if (sqlite3_prepare_v2(db_, sql.data(), -1, &stmt_, nullptr)) { - sqlite3_close(db_); throw std::runtime_error((sql + ": could not get results").data()); } BindValues(stmt_, predicate_->Bindings()); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 1b46660..c9cdce3 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -21,8 +21,11 @@ source_group("src" FILES ${TEST_SOURCES}) # Set Properties->General->Configuration Type to Application add_executable (${EXENAME} ${TEST_SOURCES} ${TEST_HEADERS}) +# The concurrency tests use std::thread, which needs the platform threading library +find_package(Threads REQUIRED) + # Properties->Linker->Input->Additional Dependencies -target_link_libraries(${EXENAME} PUBLIC sqlite_reflection gtest_main) +target_link_libraries(${EXENAME} PUBLIC sqlite_reflection gtest_main Threads::Threads) # Creates a folder "executables" and adds target project under it set_property(TARGET ${EXENAME} PROPERTY FOLDER "executables") diff --git a/tests/concurrency_test.cc b/tests/concurrency_test.cc new file mode 100644 index 0000000..10aa51b --- /dev/null +++ b/tests/concurrency_test.cc @@ -0,0 +1,141 @@ +// MIT License +// +// Copyright (c) 2026 Ioannis Kaliakatsos +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all +// copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +// SOFTWARE. + +#include "database.h" + +#include + +#include +#include +#include +#include +#include + +#include "person.h" + +using namespace sqlite_reflection; + +// These tests exercise the shared singleton connection from many threads at once. +// Before the connection access was serialized (issue #9), concurrent writers raced +// on a single sqlite3* and each operation's BEGIN/COMMIT interleaved, producing +// "cannot start a transaction within a transaction" failures and intermittent +// crashes. They deterministically pass only once access is serialized. +class ConcurrencyTest : public ::testing::Test { + void SetUp() override { + Database::Initialize(""); + } + + void TearDown() override { + Database::Finalize(); + } +}; + +TEST_F(ConcurrencyTest, ConcurrentAutoIncrementInsertsAllSucceedWithUniqueIds) { + const auto& db = Database::Instance(); + + constexpr int kThreads = 8; + constexpr int kInsertsPerThread = 100; + constexpr int kExpected = kThreads * kInsertsPerThread; + + std::atomic failures{0}; + std::vector workers; + workers.reserve(kThreads); + for (int t = 0; t < kThreads; ++t) { + workers.emplace_back([&db, &failures] { + for (int i = 0; i < kInsertsPerThread; ++i) { + try { + Person p{L"john", L"doe", 30}; + db.SaveAutoIncrement(p); + } catch (...) { + failures.fetch_add(1, std::memory_order_relaxed); + } + } + }); + } + for (auto& w : workers) { + w.join(); + } + + // No operation may fail (e.g. with a nested-transaction error). + EXPECT_EQ(0, failures.load()); + + // Every insert must be persisted exactly once. + const auto all = db.FetchAll(); + EXPECT_EQ(kExpected, static_cast(all.size())); + + // Every row must have received a distinct, database-assigned id. + std::set ids; + for (const auto& person : all) { + ids.insert(person.id); + } + EXPECT_EQ(kExpected, static_cast(ids.size())); +} + +TEST_F(ConcurrencyTest, ConcurrentReadsAndWritesDoNotThrow) { + const auto& db = Database::Instance(); + + constexpr int kWriters = 4; + constexpr int kReaders = 4; + constexpr int kInsertsPerWriter = 100; + constexpr int kReadsPerReader = 100; + constexpr int kExpected = kWriters * kInsertsPerWriter; + + std::atomic failures{0}; + std::vector workers; + workers.reserve(kWriters + kReaders); + + for (int t = 0; t < kWriters; ++t) { + workers.emplace_back([&db, &failures] { + for (int i = 0; i < kInsertsPerWriter; ++i) { + try { + Person p{L"jane", L"roe", 41}; + db.SaveAutoIncrement(p); + } catch (...) { + failures.fetch_add(1, std::memory_order_relaxed); + } + } + }); + } + + for (int t = 0; t < kReaders; ++t) { + workers.emplace_back([&db, &failures] { + for (int i = 0; i < kReadsPerReader; ++i) { + try { + // Reading concurrently with writers must not crash or throw. + volatile auto count = db.FetchAll().size(); + (void)count; + } catch (...) { + failures.fetch_add(1, std::memory_order_relaxed); + } + } + }); + } + + for (auto& w : workers) { + w.join(); + } + + EXPECT_EQ(0, failures.load()); + + const auto all = db.FetchAll(); + EXPECT_EQ(kExpected, static_cast(all.size())); +} From 697da9250ee6c91ebaa8395c0b2c7afc2424e337 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 15 Jun 2026 14:17:27 +0000 Subject: [PATCH 2/6] test: use default reference capture in concurrency lambdas MSVC in C++11 mode rejected the lambdas (C3493) because the constexpr loop-bound locals were used without being captured; C++17+ does not require capturing them, so only the C++11 jobs failed. Capture by reference so the tests build under every standard in the matrix. --- tests/concurrency_test.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/concurrency_test.cc b/tests/concurrency_test.cc index 10aa51b..c1c0806 100644 --- a/tests/concurrency_test.cc +++ b/tests/concurrency_test.cc @@ -60,7 +60,7 @@ TEST_F(ConcurrencyTest, ConcurrentAutoIncrementInsertsAllSucceedWithUniqueIds) { std::vector workers; workers.reserve(kThreads); for (int t = 0; t < kThreads; ++t) { - workers.emplace_back([&db, &failures] { + workers.emplace_back([&] { for (int i = 0; i < kInsertsPerThread; ++i) { try { Person p{L"john", L"doe", 30}; @@ -104,7 +104,7 @@ TEST_F(ConcurrencyTest, ConcurrentReadsAndWritesDoNotThrow) { workers.reserve(kWriters + kReaders); for (int t = 0; t < kWriters; ++t) { - workers.emplace_back([&db, &failures] { + workers.emplace_back([&] { for (int i = 0; i < kInsertsPerWriter; ++i) { try { Person p{L"jane", L"roe", 41}; @@ -117,7 +117,7 @@ TEST_F(ConcurrencyTest, ConcurrentReadsAndWritesDoNotThrow) { } for (int t = 0; t < kReaders; ++t) { - workers.emplace_back([&db, &failures] { + workers.emplace_back([&] { for (int i = 0; i < kReadsPerReader; ++i) { try { // Reading concurrently with writers must not crash or throw. From 7c9c547274ec3d0daa163b512f2a7917260a1683 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 15 Jun 2026 18:15:19 +0000 Subject: [PATCH 3/6] Make Instance() return a shared handle so Finalize can't free in-flight DB (#9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously Finalize() closed the connection and deleted the singleton while holding only instance_mutex_, so it could tear the Database down while another thread was mid-operation (operations hold db_mutex_, not instance_mutex_), leaving a freed sqlite3*/destroyed mutex — a use-after-free flagged in review. Move the singleton to shared ownership: - instance_ is now a std::shared_ptr, and Instance() returns std::shared_ptr. Callers hold a strong reference for the duration of their work. - Finalize() just drops the singleton's own reference; the connection is closed by a new ~Database (RAII) once the last outstanding handle is released, so an in-flight operation keeps the database alive until it completes. - Update all call sites (tests, README) to use the handle via ->, and document the threading contract and handle semantics in the README. Adds a regression test (ConnectionOutlivesFinalizeWhileAHandleIsHeld) proving a held handle stays usable after a concurrent Finalize(). --- README.md | 56 ++++++----- include/database.h | 16 +++- src/database.cc | 22 +++-- tests/concurrency_test.cc | 36 +++++-- tests/database_test.cc | 192 +++++++++++++++++++------------------- tests/date_time_test.cc | 6 +- 6 files changed, 187 insertions(+), 141 deletions(-) diff --git a/README.md b/README.md index 089941f..7d7a21b 100644 --- a/README.md +++ b/README.md @@ -49,19 +49,19 @@ int main() { // Open a persistent SQLite database file and create tables for all // reflected records that have been included in the program. Database::Initialize("people.sqlite"); - const auto& db = Database::Instance(); + const auto db = Database::Instance(); // Save a strongly typed C++ object. The last constructor argument is the id // added by sqlite-reflection to every reflected record. Person person{L"John", L"Appleseed", 42, true, 1}; - db.Save(person); + db->Save(person); // Build a WHERE predicate with member pointers instead of SQL strings. const auto adults_named_john = GreaterThanOrEqual(&Person::age, 18) .And(Equal(&Person::first_name, L"John")); // Fetch all Person rows matching the predicate. - const auto people = db.Fetch(&adults_named_john); + const auto people = db->Fetch(&adults_named_john); // Close the SQLite connection during application shutdown. Database::Finalize(); @@ -164,22 +164,34 @@ Passing an empty path creates an in-memory database, which is useful for tests: Database::Initialize(""); ``` +### Thread safety + +`Database` is safe to use from multiple threads. The shared connection is opened in +SQLite's serialized mode and all operations are serialized internally, so concurrent +`Save`/`Fetch`/`Update`/`Delete` calls are safe (they execute one at a time rather than +in parallel). `Initialize`, `Finalize`, and `Instance` are also safe to call concurrently. + +`Instance()` returns a `std::shared_ptr`; hold on to it for the duration of +your work. Because the handle keeps the database alive, an operation in progress on one +thread is never torn down by a concurrent `Finalize()` on another — the connection is closed +only once the last handle is released. + ## Saving records Use `Save` when you want to provide the `id` yourself. ```c++ -const auto& db = Database::Instance(); +const auto db = Database::Instance(); // Save one record with an explicit id. Person peter{L"Peter", L"Meier", 32, true, 5}; -db.Save(peter); +db->Save(peter); // Save multiple records in one call. std::vector people; people.push_back({L"Mary", L"Poppins", 29, false, 6}); people.push_back({L"Jane", L"Doe", 41, true, 7}); -db.Save(people); +db->Save(people); ``` Use `SaveAutoIncrement` when you want the library to assign the next available `id` for the saved row. The database generates the `id` (via an `AUTOINCREMENT` column, so values are never reused even after rows are deleted) and it is written back into the passed-in object, which must therefore be a mutable (non-`const`) lvalue. @@ -189,14 +201,14 @@ Use `SaveAutoIncrement` when you want the library to assign the next available ` ```c++ // Omit the id and let sqlite-reflection assign the next available value. Person new_person{L"John", L"Doe", 28, false}; -db.SaveAutoIncrement(new_person); +db->SaveAutoIncrement(new_person); // new_person.id now holds the value assigned by the database. // The same auto-increment behavior is available for batches. std::vector more_people; more_people.push_back({L"Ada", L"Lovelace", 36, true}); more_people.push_back({L"Grace", L"Hopper", 85, true}); -db.SaveAutoIncrement(more_people); +db->SaveAutoIncrement(more_people); // Each element's id is now populated. ``` @@ -206,14 +218,14 @@ Fetch all records of a type: ```c++ // SELECT * FROM Person; -const auto people = db.FetchAll(); +const auto people = db->FetchAll(); ``` Fetch one record by `id`: ```c++ // Fetch throws if no Person with id 5 exists. -const auto person = db.Fetch(5); +const auto person = db->Fetch(5); ``` Fetch records with a predicate: @@ -227,7 +239,7 @@ const auto predicate = GreaterThanOrEqual(&Person::id, 2) .And(Equal(&Person::first_name, L"John")); // SELECT matching Person rows and hydrate them back into C++ objects. -const auto matches = db.Fetch(&predicate); +const auto matches = db->Fetch(&predicate); ``` Available predicate helpers include: @@ -252,22 +264,22 @@ Fetch or construct a record, change its fields, and pass it to `Update`. The `id ```c++ // The row with id 5 will be replaced with these member values. -Person person = db.Fetch(5); +Person person = db->Fetch(5); person.last_name = L"Rambo"; person.age = 33; -db.Update(person); +db->Update(person); ``` Multiple records can be updated in one call: ```c++ // Update each record in the vector by its id. -std::vector people = db.FetchAll(); +std::vector people = db->FetchAll(); people[0].last_name = L"Rambo"; people[1].age = 20; -db.Update(people); +db->Update(people); ``` ## Deleting records @@ -276,15 +288,15 @@ Delete by record: ```c++ // Delete the row matching person.id. -const auto person = db.Fetch(5); -db.Delete(person); +const auto person = db->Fetch(5); +db->Delete(person); ``` Delete by `id`: ```c++ // Delete the Person row whose id is 5. -db.Delete(5); +db->Delete(5); ``` Delete by predicate: @@ -294,7 +306,7 @@ Delete by predicate: const auto predicate = SmallerThan(&Person::age, 30) .And(Equal(&Person::is_vaccinated, true)); -db.Delete(&predicate); +db->Delete(&predicate); ``` ## Date and time fields @@ -322,7 +334,7 @@ using namespace sqlite_reflection; // TimePoint(0) represents the Unix epoch. Event event{L"Created", TimePoint(0), 1}; -db.Save(event); +db->Save(event); ``` ## Raw SQL @@ -332,10 +344,10 @@ The typed CRUD API should be preferred for normal application code. It uses prep For advanced cases, `UnsafeSql` executes raw SQL text directly: ```c++ -const auto& db = Database::Instance(); +const auto db = Database::Instance(); // Executes the SQL string as-is. Use only for trusted SQL. -db.UnsafeSql("DELETE FROM Person WHERE length(first_name) <= 4"); +db->UnsafeSql("DELETE FROM Person WHERE length(first_name) <= 4"); ``` Only use `UnsafeSql` with trusted SQL strings. Do not concatenate user input into raw SQL. diff --git a/include/database.h b/include/database.h index bde0faa..d167550 100644 --- a/include/database.h +++ b/include/database.h @@ -23,6 +23,7 @@ #pragma once #include +#include #include #include #include @@ -46,12 +47,17 @@ class REFLECTION_EXPORT Database { /// in the database. If the path is empty, an in-memory database is created. static void Initialize(const std::string& path = ""); - /// This should, ideally, be called before the program finishes execution, so that - /// the database connection is closed. + /// Releases the database singleton. The underlying connection is closed once the last + /// holder of an Instance() handle has released it, so a concurrent in-flight operation + /// keeps the connection alive until it completes. static void Finalize(); - /// Retrieves the database singleton wrapper for further operations - static const Database& Instance(); + /// Retrieves the database singleton wrapper for further operations. The returned shared + /// handle keeps the database alive for as long as the caller holds it, so an operation in + /// progress is never torn down by a concurrent Finalize(). + static std::shared_ptr Instance(); + + ~Database(); Database(Database const&) = delete; Database(Database&&) = delete; @@ -236,7 +242,7 @@ class REFLECTION_EXPORT Database { /// Deletes a single record from the database void Delete(const Reflection& record, const QueryPredicateBase* predicate) const; - static Database* instance_; + static std::shared_ptr instance_; /// Guards the singleton lifecycle (Initialize / Finalize / Instance) static std::mutex instance_mutex_; diff --git a/src/database.cc b/src/database.cc index 0f0f102..1acada2 100644 --- a/src/database.cc +++ b/src/database.cc @@ -22,6 +22,7 @@ #include "database.h" +#include #include #include @@ -29,7 +30,7 @@ #include "queries.h" namespace sqlite_reflection { -Database* Database::instance_ = nullptr; +std::shared_ptr Database::instance_ = nullptr; std::mutex Database::instance_mutex_; const ReflectionRegister& GetReflectionRegister() { @@ -43,15 +44,20 @@ void Database::Initialize(const std::string& path) { } const auto effective_path = !path.empty() ? path : ":memory:"; - instance_ = new Database(effective_path.data()); + instance_ = std::shared_ptr(new Database(effective_path.data())); } void Database::Finalize() { std::lock_guard lock(instance_mutex_); - if (instance_ != nullptr) { - sqlite3_close(instance_->db_); - delete instance_; - instance_ = nullptr; + // Drop the singleton's own reference. The connection is closed by ~Database once the + // last outstanding Instance() handle is released, so an in-flight operation on another + // thread keeps the database alive until it finishes. + instance_.reset(); +} + +Database::~Database() { + if (db_ != nullptr) { + sqlite3_close(db_); } } @@ -71,12 +77,12 @@ Database::Database(const char* path) : db_(nullptr) { } } -const Database& Database::Instance() { +std::shared_ptr Database::Instance() { std::lock_guard lock(instance_mutex_); if (instance_ == nullptr) { throw std::runtime_error("Database has not been initialized; call Database::Initialize() first"); } - return *instance_; + return instance_; } FetchQueryResults Database::Fetch(const Reflection& record, const QueryPredicateBase* predicate) const { diff --git a/tests/concurrency_test.cc b/tests/concurrency_test.cc index c1c0806..cfd6504 100644 --- a/tests/concurrency_test.cc +++ b/tests/concurrency_test.cc @@ -50,7 +50,7 @@ class ConcurrencyTest : public ::testing::Test { }; TEST_F(ConcurrencyTest, ConcurrentAutoIncrementInsertsAllSucceedWithUniqueIds) { - const auto& db = Database::Instance(); + const auto db = Database::Instance(); constexpr int kThreads = 8; constexpr int kInsertsPerThread = 100; @@ -64,7 +64,7 @@ TEST_F(ConcurrencyTest, ConcurrentAutoIncrementInsertsAllSucceedWithUniqueIds) { for (int i = 0; i < kInsertsPerThread; ++i) { try { Person p{L"john", L"doe", 30}; - db.SaveAutoIncrement(p); + db->SaveAutoIncrement(p); } catch (...) { failures.fetch_add(1, std::memory_order_relaxed); } @@ -79,7 +79,7 @@ TEST_F(ConcurrencyTest, ConcurrentAutoIncrementInsertsAllSucceedWithUniqueIds) { EXPECT_EQ(0, failures.load()); // Every insert must be persisted exactly once. - const auto all = db.FetchAll(); + const auto all = db->FetchAll(); EXPECT_EQ(kExpected, static_cast(all.size())); // Every row must have received a distinct, database-assigned id. @@ -91,7 +91,7 @@ TEST_F(ConcurrencyTest, ConcurrentAutoIncrementInsertsAllSucceedWithUniqueIds) { } TEST_F(ConcurrencyTest, ConcurrentReadsAndWritesDoNotThrow) { - const auto& db = Database::Instance(); + const auto db = Database::Instance(); constexpr int kWriters = 4; constexpr int kReaders = 4; @@ -108,7 +108,7 @@ TEST_F(ConcurrencyTest, ConcurrentReadsAndWritesDoNotThrow) { for (int i = 0; i < kInsertsPerWriter; ++i) { try { Person p{L"jane", L"roe", 41}; - db.SaveAutoIncrement(p); + db->SaveAutoIncrement(p); } catch (...) { failures.fetch_add(1, std::memory_order_relaxed); } @@ -121,7 +121,7 @@ TEST_F(ConcurrencyTest, ConcurrentReadsAndWritesDoNotThrow) { for (int i = 0; i < kReadsPerReader; ++i) { try { // Reading concurrently with writers must not crash or throw. - volatile auto count = db.FetchAll().size(); + volatile auto count = db->FetchAll().size(); (void)count; } catch (...) { failures.fetch_add(1, std::memory_order_relaxed); @@ -136,6 +136,28 @@ TEST_F(ConcurrencyTest, ConcurrentReadsAndWritesDoNotThrow) { EXPECT_EQ(0, failures.load()); - const auto all = db.FetchAll(); + const auto all = db->FetchAll(); EXPECT_EQ(kExpected, static_cast(all.size())); } + +TEST_F(ConcurrencyTest, ConnectionOutlivesFinalizeWhileAHandleIsHeld) { + // A caller holding an Instance() handle must be able to keep using the database even if + // another thread calls Finalize() in the meantime: the connection is reference-counted + // and only closed once the last handle is released. With the previous raw-pointer + // lifecycle, Finalize() deleted the object and closed the connection immediately, so the + // operations below would have been a use-after-free. + auto db = Database::Instance(); + + Person first{L"ada", L"lovelace", 36}; + db->SaveAutoIncrement(first); + + // Drop the singleton's own reference while we still hold one. + Database::Finalize(); + + // The held handle must still be valid: the connection is alive and usable. If Finalize() + // had closed/freed it, these calls would crash or throw. + Person second{L"grace", L"hopper", 85}; + db->SaveAutoIncrement(second); + const auto all = db->FetchAll(); + EXPECT_EQ(2, static_cast(all.size())); +} diff --git a/tests/database_test.cc b/tests/database_test.cc index 1265875..ca143f1 100644 --- a/tests/database_test.cc +++ b/tests/database_test.cc @@ -42,22 +42,22 @@ class DatabaseTest : public ::testing::Test { }; TEST_F(DatabaseTest, Initialization) { - const auto& db = Database::Instance(); + const auto db = Database::Instance(); - const auto all_persons = db.FetchAll(); + const auto all_persons = db->FetchAll(); EXPECT_EQ(0, all_persons.size()); - const auto all_pets = db.FetchAll(); + const auto all_pets = db->FetchAll(); EXPECT_EQ(0, all_pets.size()); } TEST_F(DatabaseTest, SingleInsertion) { - const auto& db = Database::Instance(); + const auto db = Database::Instance(); const Person p{L"παναγιώτης", L"ανδριανόπουλος", 39, 1}; - db.Save(p); + db->Save(p); - const auto all_persons = db.FetchAll(); + const auto all_persons = db->FetchAll(); EXPECT_EQ(1, all_persons.size()); EXPECT_EQ(p.first_name, all_persons[0].first_name); @@ -67,15 +67,15 @@ TEST_F(DatabaseTest, SingleInsertion) { } TEST_F(DatabaseTest, SingleInsertionWithAutoIdIncrement) { - const auto& db = Database::Instance(); + const auto db = Database::Instance(); Person p{L"παναγιώτης", L"ανδριανόπουλος", 39}; - db.SaveAutoIncrement(p); + db->SaveAutoIncrement(p); // The generated id is written back into the passed-in record EXPECT_EQ(1, p.id); - const auto all_persons = db.FetchAll(); + const auto all_persons = db->FetchAll(); EXPECT_EQ(1, all_persons.size()); EXPECT_EQ(p.first_name, all_persons[0].first_name); @@ -85,19 +85,19 @@ TEST_F(DatabaseTest, SingleInsertionWithAutoIdIncrement) { } TEST_F(DatabaseTest, MultipleInsertionsWithAutoIdIncrement) { - const auto& db = Database::Instance(); + const auto db = Database::Instance(); std::vector persons; persons.push_back({L"παναγιώτης", L"ανδριανόπουλος", 28}); persons.push_back({L"peter", L"meier", 32}); - db.SaveAutoIncrement(persons); + db->SaveAutoIncrement(persons); // The generated ids are written back into the passed-in records EXPECT_EQ(1, persons[0].id); EXPECT_EQ(2, persons[1].id); - const auto saved_persons = db.FetchAll(); + const auto saved_persons = db->FetchAll(); EXPECT_EQ(2, saved_persons.size()); for (auto i = 0; i < saved_persons.size(); ++i) { @@ -109,68 +109,68 @@ TEST_F(DatabaseTest, MultipleInsertionsWithAutoIdIncrement) { } TEST_F(DatabaseTest, AutoIdIncrementContinuesFromExistingMaxId) { - const auto& db = Database::Instance(); + const auto db = Database::Instance(); const Person existing{L"ada", L"lovelace", 36, true, 10}; - db.Save(existing); + db->Save(existing); Person p{L"grace", L"hopper", 85}; - db.SaveAutoIncrement(p); + db->SaveAutoIncrement(p); // The new id continues from the current maximum id in the table EXPECT_EQ(11, p.id); } TEST_F(DatabaseTest, AutoIdIsNotReusedAfterDeletingHighestRow) { - const auto& db = Database::Instance(); + const auto db = Database::Instance(); Person first{L"ada", L"lovelace", 36}; - db.SaveAutoIncrement(first); + db->SaveAutoIncrement(first); EXPECT_EQ(1, first.id); Person second{L"grace", L"hopper", 85}; - db.SaveAutoIncrement(second); + db->SaveAutoIncrement(second); EXPECT_EQ(2, second.id); // Remove the row with the highest id - db.Delete(second); + db->Delete(second); // A subsequent auto-increment save must not reuse the freed id Person third{L"john", L"doe", 28}; - db.SaveAutoIncrement(third); + db->SaveAutoIncrement(third); EXPECT_EQ(3, third.id); } TEST_F(DatabaseTest, AutoIncrementInsertForIdOnlyRecord) { - const auto& db = Database::Instance(); + const auto db = Database::Instance(); // A record whose only column is the implicit id must still insert via the // auto-increment path (INSERT INTO ... DEFAULT VALUES) and get an assigned id IdOnlyRecord first; - db.SaveAutoIncrement(first); + db->SaveAutoIncrement(first); EXPECT_EQ(1, first.id); IdOnlyRecord second; - db.SaveAutoIncrement(second); + db->SaveAutoIncrement(second); EXPECT_EQ(2, second.id); - const auto all = db.FetchAll(); + const auto all = db->FetchAll(); EXPECT_EQ(2, all.size()); EXPECT_EQ(1, all[0].id); EXPECT_EQ(2, all[1].id); } TEST_F(DatabaseTest, MultipleInsertions) { - const auto& db = Database::Instance(); + const auto db = Database::Instance(); std::vector persons; persons.push_back({L"παναγιώτης", L"ανδριανόπουλος", 28, false, 3}); persons.push_back({L"peter", L"meier", 32, true, 5}); - db.Save(persons); + db->Save(persons); - const auto saved_persons = db.FetchAll(); + const auto saved_persons = db->FetchAll(); EXPECT_EQ(2, saved_persons.size()); for (auto i = 0; i < saved_persons.size(); ++i) { @@ -183,27 +183,27 @@ TEST_F(DatabaseTest, MultipleInsertions) { } TEST_F(DatabaseTest, InsertionOnOneTypeDoesNotAffectOtherType) { - const auto& db = Database::Instance(); + const auto db = Database::Instance(); const Person p{L"παναγιώτης", L"ανδριανόπουλος", 39, 1}; - db.Save(p); + db->Save(p); - const auto all_pets = db.FetchAll(); + const auto all_pets = db->FetchAll(); EXPECT_EQ(0, all_pets.size()); } TEST_F(DatabaseTest, SingleUpdate) { - const auto& db = Database::Instance(); + const auto db = Database::Instance(); Person p{L"παναγιώτης", L"ανδριανόπουλος", 39, 1}; - db.Save(p); + db->Save(p); p.age = 23; p.first_name = L"max"; - db.Update(p); + db->Update(p); - const auto all_persons = db.FetchAll(); + const auto all_persons = db->FetchAll(); EXPECT_EQ(1, all_persons.size()); EXPECT_EQ(p.first_name, all_persons[0].first_name); @@ -213,21 +213,21 @@ TEST_F(DatabaseTest, SingleUpdate) { } TEST_F(DatabaseTest, MultipleUpdates) { - const auto& db = Database::Instance(); + const auto db = Database::Instance(); std::vector persons; persons.push_back({L"john", L"doe", 28, false, 3}); persons.push_back({L"mary", L"poppins", 29, false, 5}); - db.Save(persons); + db->Save(persons); persons[0].last_name = L"rambo"; persons[1].age = 20; - db.Update(persons); + db->Update(persons); - const auto saved_persons = db.FetchAll(); + const auto saved_persons = db->FetchAll(); EXPECT_EQ(2, saved_persons.size()); for (auto i = 0; i < saved_persons.size(); ++i) { @@ -239,12 +239,12 @@ TEST_F(DatabaseTest, MultipleUpdates) { } TEST_F(DatabaseTest, InsertedTextSqlPayloadIsPersistedLiterally) { - const auto& db = Database::Instance(); + const auto db = Database::Instance(); const Person p{L"john'); DROP TABLE Person; --", L"payload", 39, false, 1}; - db.Save(p); + db->Save(p); - const auto all_persons = db.FetchAll(); + const auto all_persons = db->FetchAll(); EXPECT_EQ(1, all_persons.size()); EXPECT_EQ(p.first_name, all_persons[0].first_name); EXPECT_EQ(p.last_name, all_persons[0].last_name); @@ -253,32 +253,32 @@ TEST_F(DatabaseTest, InsertedTextSqlPayloadIsPersistedLiterally) { } TEST_F(DatabaseTest, InsertedTextWithEmbeddedNulIsPersistedLiterally) { - const auto& db = Database::Instance(); + const auto db = Database::Instance(); const Person p{std::wstring(L"a\0b", 3), L"payload", 39, false, 1}; - db.Save(p); + db->Save(p); - const auto all_persons = db.FetchAll(); + const auto all_persons = db->FetchAll(); ASSERT_EQ(1, all_persons.size()); EXPECT_EQ(p.first_name, all_persons[0].first_name); EXPECT_EQ(3, all_persons[0].first_name.size()); const auto predicate = Equal(&Person::first_name, std::wstring(L"a\0b", 3)); - const auto matching_persons = db.Fetch(&predicate); + const auto matching_persons = db->Fetch(&predicate); ASSERT_EQ(1, matching_persons.size()); EXPECT_EQ(p.first_name, matching_persons[0].first_name); } TEST_F(DatabaseTest, UpdatedTextSqlPayloadIsPersistedLiterally) { - const auto& db = Database::Instance(); + const auto db = Database::Instance(); Person p{L"john", L"payload", 39, false, 1}; - db.Save(p); + db->Save(p); p.first_name = L"updated'); DELETE FROM Person; --"; - db.Update(p); + db->Update(p); - const auto all_persons = db.FetchAll(); + const auto all_persons = db->FetchAll(); EXPECT_EQ(1, all_persons.size()); EXPECT_EQ(p.first_name, all_persons[0].first_name); EXPECT_EQ(p.last_name, all_persons[0].last_name); @@ -287,7 +287,7 @@ TEST_F(DatabaseTest, UpdatedTextSqlPayloadIsPersistedLiterally) { } TEST_F(DatabaseTest, DeleteWithRecord) { - const auto& db = Database::Instance(); + const auto db = Database::Instance(); std::vector persons; @@ -295,13 +295,13 @@ TEST_F(DatabaseTest, DeleteWithRecord) { persons.push_back({L"peter", L"meier", 32, false, 5}); persons.push_back({L"mary", L"poppins", 20, false, 13}); - db.Save(persons); + db->Save(persons); - auto saved_persons = db.FetchAll(); + auto saved_persons = db->FetchAll(); EXPECT_EQ(3, saved_persons.size()); - db.Delete(persons[1]); - saved_persons = db.FetchAll(); + db->Delete(persons[1]); + saved_persons = db->FetchAll(); EXPECT_EQ(2, saved_persons.size()); auto i = 0; @@ -318,7 +318,7 @@ TEST_F(DatabaseTest, DeleteWithRecord) { } TEST_F(DatabaseTest, DeleteWithId) { - const auto& db = Database::Instance(); + const auto db = Database::Instance(); std::vector persons; @@ -326,13 +326,13 @@ TEST_F(DatabaseTest, DeleteWithId) { persons.push_back({L"peter", L"meier", 32, false, 5}); persons.push_back({L"mary", L"poppins", 20, false, 13}); - db.Save(persons); + db->Save(persons); - auto saved_persons = db.FetchAll(); + auto saved_persons = db->FetchAll(); EXPECT_EQ(3, saved_persons.size()); - db.Delete(persons[1].id); - saved_persons = db.FetchAll(); + db->Delete(persons[1].id); + saved_persons = db->FetchAll(); EXPECT_EQ(2, saved_persons.size()); auto i = 0; @@ -349,7 +349,7 @@ TEST_F(DatabaseTest, DeleteWithId) { } TEST_F(DatabaseTest, DeleteWithPredicate) { - const auto& db = Database::Instance(); + const auto db = Database::Instance(); std::vector persons; @@ -357,12 +357,12 @@ TEST_F(DatabaseTest, DeleteWithPredicate) { persons.push_back({L"peter", L"meier", 32, false, 5}); persons.push_back({L"mary", L"poppins", 20, true, 13}); - db.Save(persons); + db->Save(persons); const auto age_match_predicate = SmallerThan(&Person::age, 30).And(Equal(&Person::is_vaccinated, true)); - db.Delete(&age_match_predicate); - const auto fetched_persons = db.FetchAll(); + db->Delete(&age_match_predicate); + const auto fetched_persons = db->FetchAll(); EXPECT_EQ(1, fetched_persons.size()); EXPECT_EQ(5, fetched_persons[0].id); @@ -373,24 +373,24 @@ TEST_F(DatabaseTest, DeleteWithPredicate) { } TEST_F(DatabaseTest, DeleteWithInjectedPredicateDoesNotDeleteRows) { - const auto& db = Database::Instance(); + const auto db = Database::Instance(); std::vector persons; persons.push_back({L"john", L"doe", 28, false, 3}); persons.push_back({L"mary", L"poppins", 20, false, 5}); - db.Save(persons); + db->Save(persons); const auto injected_predicate = Equal(&Person::first_name, L"nobody' OR 1=1 --"); - db.Delete(&injected_predicate); + db->Delete(&injected_predicate); - const auto fetched_persons = db.FetchAll(); + const auto fetched_persons = db->FetchAll(); EXPECT_EQ(2, fetched_persons.size()); } TEST_F(DatabaseTest, SingleFetch) { - const auto& db = Database::Instance(); + const auto db = Database::Instance(); std::vector persons; @@ -398,9 +398,9 @@ TEST_F(DatabaseTest, SingleFetch) { persons.push_back({L"peter", L"meier", 32, false, 5}); persons.push_back({L"mary", L"poppins", 20, false, 13}); - db.Save(persons); + db->Save(persons); - const auto fetched_person = db.Fetch(5); + const auto fetched_person = db->Fetch(5); EXPECT_EQ(5, fetched_person.id); EXPECT_EQ(L"peter", fetched_person.first_name); EXPECT_EQ(L"meier", fetched_person.last_name); @@ -408,7 +408,7 @@ TEST_F(DatabaseTest, SingleFetch) { } TEST_F(DatabaseTest, SingleFetchWithoutExistingRecordExpectingException) { - const auto& db = Database::Instance(); + const auto db = Database::Instance(); std::vector persons; @@ -416,28 +416,28 @@ TEST_F(DatabaseTest, SingleFetchWithoutExistingRecordExpectingException) { persons.push_back({L"peter", L"meier", 32, false, 5}); persons.push_back({L"mary", L"poppins", 20, false}); - db.Save(persons); + db->Save(persons); - EXPECT_ANY_THROW(db.Fetch(15)); + EXPECT_ANY_THROW(db->Fetch(15)); } TEST_F(DatabaseTest, FetchWithInjectedPredicateDoesNotMatchAllRows) { - const auto& db = Database::Instance(); + const auto db = Database::Instance(); std::vector persons; persons.push_back({L"john", L"doe", 28, false, 3}); persons.push_back({L"mary", L"poppins", 20, false, 5}); - db.Save(persons); + db->Save(persons); const auto injected_predicate = Equal(&Person::first_name, L"john' OR 1=1 --"); - const auto fetched_persons = db.Fetch(&injected_predicate); + const auto fetched_persons = db->Fetch(&injected_predicate); EXPECT_EQ(0, fetched_persons.size()); } TEST_F(DatabaseTest, FetchWithSimilarPredicateString) { - const auto& db = Database::Instance(); + const auto db = Database::Instance(); std::vector company; @@ -449,11 +449,11 @@ TEST_F(DatabaseTest, FetchWithSimilarPredicateString) { company.push_back({L"Kim", 22, L"South-Hall", 45000.0, 6}); company.push_back({L"Janes", 24, L"Houston", 10000.0, 7}); - db.Save(company); + db->Save(company); auto fetch_condition = Like(&Company::address, L"-"); - auto fetched_persons = db.Fetch(&fetch_condition); + auto fetched_persons = db->Fetch(&fetch_condition); EXPECT_EQ(2, fetched_persons.size()); EXPECT_EQ(4, fetched_persons[0].id); @@ -470,7 +470,7 @@ TEST_F(DatabaseTest, FetchWithSimilarPredicateString) { } TEST_F(DatabaseTest, FetchWithSimilarPredicateDouble) { - const auto& db = Database::Instance(); + const auto db = Database::Instance(); std::vector company; @@ -482,11 +482,11 @@ TEST_F(DatabaseTest, FetchWithSimilarPredicateDouble) { company.push_back({L"Kim", 22, L"South-Hall", 45000.0, 6}); company.push_back({L"Janes", 24, L"Houston", 10000.0, 7}); - db.Save(company); + db->Save(company); auto fetch_condition = Like(&Company::salary, 5000.0); - auto fetched_persons = db.Fetch(&fetch_condition); + auto fetched_persons = db->Fetch(&fetch_condition); EXPECT_EQ(4, fetched_persons.size()); EXPECT_EQ(2, fetched_persons[0].id); @@ -515,7 +515,7 @@ TEST_F(DatabaseTest, FetchWithSimilarPredicateDouble) { } TEST_F(DatabaseTest, FetchWithSimilarPredicateInt) { - const auto& db = Database::Instance(); + const auto db = Database::Instance(); std::vector company; @@ -527,11 +527,11 @@ TEST_F(DatabaseTest, FetchWithSimilarPredicateInt) { company.push_back({L"Kim", 22, L"South-Hall", 45000.0, 6}); company.push_back({L"Janes", 24, L"Houston", 10000.0, 7}); - db.Save(company); + db->Save(company); const auto fetch_condition = Like(&Company::age, 7); - const auto fetched_persons = db.Fetch(&fetch_condition); + const auto fetched_persons = db->Fetch(&fetch_condition); EXPECT_EQ(1, fetched_persons.size()); EXPECT_EQ(5, fetched_persons[0].id); @@ -542,7 +542,7 @@ TEST_F(DatabaseTest, FetchWithSimilarPredicateInt) { } TEST_F(DatabaseTest, FetchWithPredicateChaining) { - const auto& db = Database::Instance(); + const auto db = Database::Instance(); std::vector persons; @@ -552,12 +552,12 @@ TEST_F(DatabaseTest, FetchWithPredicateChaining) { persons.push_back({L"jame", L"surname4", 45, false, 4}); persons.push_back({L"name5", L"surname5", 56, false, 5}); - db.Save(persons); + db->Save(persons); const auto fetch_condition = GreaterThanOrEqual(&Person::id, 2).And(SmallerThan(&Person::id, 5)).And(Equal(&Person::first_name, L"john")); - const auto fetched_persons = db.Fetch(&fetch_condition); + const auto fetched_persons = db->Fetch(&fetch_condition); EXPECT_EQ(2, fetched_persons.size()); EXPECT_EQ(2, fetched_persons[0].id); @@ -572,34 +572,34 @@ TEST_F(DatabaseTest, FetchWithPredicateChaining) { } TEST_F(DatabaseTest, ReadMaxId) { - const auto& db = Database::Instance(); + const auto db = Database::Instance(); std::vector persons; persons.push_back({L"john", L"appleseed", 28, false, 54}); persons.push_back({L"mary", L"poppins", 20, false, 156}); - db.Save(persons); + db->Save(persons); - const auto max_id_person = db.GetMaxId(); + const auto max_id_person = db->GetMaxId(); EXPECT_EQ(156, max_id_person); - const auto max_id_pet = db.GetMaxId(); + const auto max_id_pet = db->GetMaxId(); EXPECT_EQ(0, max_id_pet); } TEST_F(DatabaseTest, RawSqlQueryForPersistedRecord) { - const auto& db = Database::Instance(); + const auto db = Database::Instance(); std::vector persons; persons.push_back({L"johnie", L"appleseed", 28, false, 52}); persons.push_back({L"mary", L"poppins", 20, false, 156}); - db.Save(persons); - db.UnsafeSql("DELETE FROM Person WHERE length(first_name) <= 4"); + db->Save(persons); + db->UnsafeSql("DELETE FROM Person WHERE length(first_name) <= 4"); - const auto fetched_persons = db.FetchAll(); + const auto fetched_persons = db->FetchAll(); EXPECT_EQ(1, fetched_persons.size()); EXPECT_EQ(52, fetched_persons[0].id); EXPECT_EQ(L"johnie", fetched_persons[0].first_name); diff --git a/tests/date_time_test.cc b/tests/date_time_test.cc index 70a58c3..a58e925 100644 --- a/tests/date_time_test.cc +++ b/tests/date_time_test.cc @@ -49,10 +49,10 @@ void ControlRoundTrip(const int64_t& time_point, const wchar_t* sys_time) { f.id = 0; f.creation_date = TimePoint(time_point); - const auto& db = Database::Instance(); - db.Save(f); + const auto db = Database::Instance(); + db->Save(f); - const auto retrieved = db.FetchAll(); + const auto retrieved = db->FetchAll(); EXPECT_EQ(1, retrieved.size()); auto sys_time_actual = retrieved[0].creation_date.SystemTime(); From 7ffbdf8fe3bcc4d35c592f976bffc697c6e5c224 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 15 Jun 2026 18:22:13 +0000 Subject: [PATCH 4/6] docs: clarify the threading and ownership model in the README Make the shared-ownership contract concrete: explain that Instance() returns a shared_ptr handle that keeps the database alive, that Finalize() only drops the singleton's own handle (the connection closes when the last handle is released), and add a worker-thread example showing a handle captured by value. Also correct the Finalize() comment in the open/close section, which implied the connection is closed immediately. --- README.md | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 7d7a21b..c450e47 100644 --- a/README.md +++ b/README.md @@ -152,7 +152,8 @@ void StartApplication(const std::string& db_path) { } void StopApplication() { - // Finalize releases the singleton database connection. + // Finalize releases the singleton's own handle. The underlying connection is closed once + // the last outstanding Instance() handle is released (see "Thread safety" below). Database::Finalize(); } ``` @@ -171,10 +172,25 @@ SQLite's serialized mode and all operations are serialized internally, so concur `Save`/`Fetch`/`Update`/`Delete` calls are safe (they execute one at a time rather than in parallel). `Initialize`, `Finalize`, and `Instance` are also safe to call concurrently. -`Instance()` returns a `std::shared_ptr`; hold on to it for the duration of -your work. Because the handle keeps the database alive, an operation in progress on one -thread is never torn down by a concurrent `Finalize()` on another — the connection is closed -only once the last handle is released. +**Ownership model.** `Instance()` returns a `std::shared_ptr` rather than a +reference. Each user holds a strong handle, so the database stays alive for as long as anyone +is using it: an operation in progress on one thread is never torn down by a concurrent +`Finalize()` on another. `Finalize()` only drops the singleton's own handle; the connection is +closed by the destructor once the last handle is released. Hold the handle for the duration of +your work (capture it by value into worker threads) rather than re-fetching it per call: + +```c++ +// Each worker captures its own handle by value, keeping the database alive while it runs. +auto db = Database::Instance(); +std::thread worker([db] { + Person p{L"Ada", L"Lovelace", 36}; + db->SaveAutoIncrement(p); // safe even if another thread calls Finalize() meanwhile +}); +worker.join(); +``` + +Writes (and reads) are serialized, so they are safe but not concurrent; true write parallelism +is tracked as future work. ## Saving records From 119b0b845d7dcbef804284960f7cb950308e5bc4 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 15 Jun 2026 18:30:03 +0000 Subject: [PATCH 5/6] Reject reinitialization while handles to the previous database are alive (#9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With the shared-handle lifecycle, Finalize() nulls instance_ but an outstanding Instance() handle can keep the previous Database (and its connection) alive. A subsequent Initialize() would then create a second live Database whose operations use a different db_mutex_ — two connections to the same file racing, or a fresh empty in-memory DB shadowing the old one. Track the retiring instance with a weak_ptr and make Initialize() throw if it is still alive, so reinitialization is only allowed once all handles to the previous database have been released. Normal init/finalize cycles (no outstanding handles, e.g. the test fixtures) are unaffected. Adds a regression test covering reject-then-allow once the handle is released. --- include/database.h | 6 ++++++ src/database.cc | 12 +++++++++++- tests/concurrency_test.cc | 17 +++++++++++++++++ 3 files changed, 34 insertions(+), 1 deletion(-) diff --git a/include/database.h b/include/database.h index d167550..93086c1 100644 --- a/include/database.h +++ b/include/database.h @@ -244,6 +244,12 @@ class REFLECTION_EXPORT Database { static std::shared_ptr instance_; + /// Observes the most recently retired instance. After Finalize() drops the singleton's + /// own reference, this stays non-expired for as long as any outstanding Instance() handle + /// keeps the previous database alive, which blocks reinitialization until those handles + /// are released (so two live databases/connections can never coexist). + static std::weak_ptr retired_; + /// Guards the singleton lifecycle (Initialize / Finalize / Instance) static std::mutex instance_mutex_; diff --git a/src/database.cc b/src/database.cc index 1acada2..ca57e81 100644 --- a/src/database.cc +++ b/src/database.cc @@ -31,6 +31,7 @@ namespace sqlite_reflection { std::shared_ptr Database::instance_ = nullptr; +std::weak_ptr Database::retired_; std::mutex Database::instance_mutex_; const ReflectionRegister& GetReflectionRegister() { @@ -42,6 +43,13 @@ void Database::Initialize(const std::string& path) { if (instance_ != nullptr) { throw std::invalid_argument("Database has already been initialized"); } + if (!retired_.expired()) { + // A previous database is still kept alive by outstanding Instance() handles. + // Creating a new one now would leave two live databases/connections that do not + // share the same db_mutex_; refuse until those handles are released. + throw std::runtime_error( + "Database cannot be reinitialized while handles to the previous database are still in use"); + } const auto effective_path = !path.empty() ? path : ":memory:"; instance_ = std::shared_ptr(new Database(effective_path.data())); @@ -51,7 +59,9 @@ void Database::Finalize() { std::lock_guard lock(instance_mutex_); // Drop the singleton's own reference. The connection is closed by ~Database once the // last outstanding Instance() handle is released, so an in-flight operation on another - // thread keeps the database alive until it finishes. + // thread keeps the database alive until it finishes. Track the retiring instance so a + // subsequent Initialize() is rejected until those handles are gone. + retired_ = instance_; instance_.reset(); } diff --git a/tests/concurrency_test.cc b/tests/concurrency_test.cc index cfd6504..7a951db 100644 --- a/tests/concurrency_test.cc +++ b/tests/concurrency_test.cc @@ -161,3 +161,20 @@ TEST_F(ConcurrencyTest, ConnectionOutlivesFinalizeWhileAHandleIsHeld) { const auto all = db->FetchAll(); EXPECT_EQ(2, static_cast(all.size())); } + +TEST_F(ConcurrencyTest, ReinitializeWhileAHandleIsHeldIsRejected) { + // Hold a handle to the current database, then finalize. The database stays alive through + // the handle, so re-initializing now would create a second live database/connection that + // does not share the first's lock. That must be rejected until the handle is released. + auto db = Database::Instance(); + Database::Finalize(); + + EXPECT_THROW(Database::Initialize(""), std::runtime_error); + + // Once the last handle is gone, the previous database is destroyed and re-initialization + // is allowed again. + db.reset(); + Database::Initialize(""); + const auto fresh = Database::Instance(); + EXPECT_EQ(0, static_cast(fresh->FetchAll().size())); +} From b7a3fa996b2f47033bb1ec5e0d6660bf43dde3dd Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 15 Jun 2026 18:33:26 +0000 Subject: [PATCH 6/6] Don't clear the reinitialization guard on repeated Finalize (#9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finalize() unconditionally did `retired_ = instance_; instance_.reset();`. On a second Finalize() (instance_ already null) this overwrote a still-valid retired_ weak_ptr with an empty one, so a later Initialize() would pass the expired() check and create a second live database while outstanding handles from the first were still usable — reopening the split-lock/two-database hole the guard prevents. Make Finalize() a no-op when there is no instance to retire, preserving the existing retired_ tracking. Adds a regression test for repeated Finalize while a handle is held. --- src/database.cc | 6 ++++++ tests/concurrency_test.cc | 16 ++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/src/database.cc b/src/database.cc index ca57e81..331f53d 100644 --- a/src/database.cc +++ b/src/database.cc @@ -57,6 +57,12 @@ void Database::Initialize(const std::string& path) { void Database::Finalize() { std::lock_guard lock(instance_mutex_); + if (instance_ == nullptr) { + // Nothing to retire. Crucially, do not overwrite retired_ here: a previous Finalize() + // may have retired an instance that outstanding handles still keep alive, and that + // guard must remain in effect until those handles are released. + return; + } // Drop the singleton's own reference. The connection is closed by ~Database once the // last outstanding Instance() handle is released, so an in-flight operation on another // thread keeps the database alive until it finishes. Track the retiring instance so a diff --git a/tests/concurrency_test.cc b/tests/concurrency_test.cc index 7a951db..9fc9a02 100644 --- a/tests/concurrency_test.cc +++ b/tests/concurrency_test.cc @@ -178,3 +178,19 @@ TEST_F(ConcurrencyTest, ReinitializeWhileAHandleIsHeldIsRejected) { const auto fresh = Database::Instance(); EXPECT_EQ(0, static_cast(fresh->FetchAll().size())); } + +TEST_F(ConcurrencyTest, RepeatedFinalizeDoesNotClearTheReinitializationGuard) { + // Calling Finalize() more than once while a handle to the first database is still held + // must not drop the guard: the second Finalize() (with instance_ already null) must leave + // the retired-handle tracking intact so Initialize() stays blocked. + auto db = Database::Instance(); + Database::Finalize(); + Database::Finalize(); // second finalize must be a no-op for the guard + + EXPECT_THROW(Database::Initialize(""), std::runtime_error); + + db.reset(); + Database::Initialize(""); + const auto fresh = Database::Instance(); + EXPECT_EQ(0, static_cast(fresh->FetchAll().size())); +}