From c8fddbd3c892e6cff682705d938522d850c75c08 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Mon, 5 Aug 2024 23:37:13 -0400 Subject: [PATCH] sqlite: ensure statement finalization on db close MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit adds statement tracking to the DatabaseSync class. When a database is closed manually or via garbage collection, it will force all associated prepared statements to be finalized. This should mitigate "zombie" connections which can introduce test flakiness in the CI on Windows. PR-URL: https://github.com/nodejs/node/pull/54014 Fixes: https://github.com/nodejs/node/issues/54006 Reviewed-By: Moshe Atlow Reviewed-By: Tobias Nießen Reviewed-By: Matteo Collina Reviewed-By: Yagiz Nizipli Reviewed-By: Richard Lau Reviewed-By: Michaël Zasso --- src/node_sqlite.cc | 102 +++++++++++++++++++++++++++++++++++---------- src/node_sqlite.h | 16 +++++-- 2 files changed, 92 insertions(+), 26 deletions(-) diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 1d94363bf089074..4821db5303501a8 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -91,8 +91,11 @@ DatabaseSync::DatabaseSync(Environment* env, } DatabaseSync::~DatabaseSync() { - sqlite3_close_v2(connection_); - connection_ = nullptr; + if (IsOpen()) { + FinalizeStatements(); + sqlite3_close_v2(connection_); + connection_ = nullptr; + } } void DatabaseSync::MemoryInfo(MemoryTracker* tracker) const { @@ -100,7 +103,7 @@ void DatabaseSync::MemoryInfo(MemoryTracker* tracker) const { } bool DatabaseSync::Open() { - if (connection_ != nullptr) { + if (IsOpen()) { node::THROW_ERR_INVALID_STATE(env(), "database is already open"); return false; } @@ -112,6 +115,29 @@ bool DatabaseSync::Open() { return true; } +void DatabaseSync::FinalizeStatements() { + for (auto stmt : statements_) { + stmt->Finalize(); + } + + statements_.clear(); +} + +void DatabaseSync::UntrackStatement(StatementSync* statement) { + auto it = statements_.find(statement); + if (it != statements_.end()) { + statements_.erase(it); + } +} + +inline bool DatabaseSync::IsOpen() { + return connection_ != nullptr; +} + +inline sqlite3* DatabaseSync::Connection() { + return connection_; +} + void DatabaseSync::New(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -164,8 +190,8 @@ void DatabaseSync::Close(const FunctionCallbackInfo& args) { DatabaseSync* db; ASSIGN_OR_RETURN_UNWRAP(&db, args.This()); Environment* env = Environment::GetCurrent(args); - THROW_AND_RETURN_ON_BAD_STATE( - env, db->connection_ == nullptr, "database is not open"); + THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open"); + db->FinalizeStatements(); int r = sqlite3_close_v2(db->connection_); CHECK_ERROR_OR_THROW(env->isolate(), db->connection_, r, SQLITE_OK, void()); db->connection_ = nullptr; @@ -175,8 +201,7 @@ void DatabaseSync::Prepare(const FunctionCallbackInfo& args) { DatabaseSync* db; ASSIGN_OR_RETURN_UNWRAP(&db, args.This()); Environment* env = Environment::GetCurrent(args); - THROW_AND_RETURN_ON_BAD_STATE( - env, db->connection_ == nullptr, "database is not open"); + THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open"); if (!args[0]->IsString()) { node::THROW_ERR_INVALID_ARG_TYPE(env->isolate(), @@ -188,8 +213,8 @@ void DatabaseSync::Prepare(const FunctionCallbackInfo& args) { sqlite3_stmt* s = nullptr; int r = sqlite3_prepare_v2(db->connection_, *sql, -1, &s, 0); CHECK_ERROR_OR_THROW(env->isolate(), db->connection_, r, SQLITE_OK, void()); - BaseObjectPtr stmt = - StatementSync::Create(env, db->connection_, s); + BaseObjectPtr stmt = StatementSync::Create(env, db, s); + db->statements_.insert(stmt.get()); args.GetReturnValue().Set(stmt->object()); } @@ -197,8 +222,7 @@ void DatabaseSync::Exec(const FunctionCallbackInfo& args) { DatabaseSync* db; ASSIGN_OR_RETURN_UNWRAP(&db, args.This()); Environment* env = Environment::GetCurrent(args); - THROW_AND_RETURN_ON_BAD_STATE( - env, db->connection_ == nullptr, "database is not open"); + THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open"); if (!args[0]->IsString()) { node::THROW_ERR_INVALID_ARG_TYPE(env->isolate(), @@ -213,7 +237,7 @@ void DatabaseSync::Exec(const FunctionCallbackInfo& args) { StatementSync::StatementSync(Environment* env, Local object, - sqlite3* db, + DatabaseSync* db, sqlite3_stmt* stmt) : BaseObject(env, object) { MakeWeak(); @@ -227,13 +251,25 @@ StatementSync::StatementSync(Environment* env, } StatementSync::~StatementSync() { + if (!IsFinalized()) { + db_->UntrackStatement(this); + Finalize(); + } +} + +void StatementSync::Finalize() { sqlite3_finalize(statement_); statement_ = nullptr; } +inline bool StatementSync::IsFinalized() { + return statement_ == nullptr; +} + bool StatementSync::BindParams(const FunctionCallbackInfo& args) { int r = sqlite3_clear_bindings(statement_); - CHECK_ERROR_OR_THROW(env()->isolate(), db_, r, SQLITE_OK, false); + CHECK_ERROR_OR_THROW( + env()->isolate(), db_->Connection(), r, SQLITE_OK, false); int anon_idx = 1; int anon_start = 0; @@ -364,7 +400,8 @@ bool StatementSync::BindValue(const Local& value, const int index) { return false; } - CHECK_ERROR_OR_THROW(env()->isolate(), db_, r, SQLITE_OK, false); + CHECK_ERROR_OR_THROW( + env()->isolate(), db_->Connection(), r, SQLITE_OK, false); return true; } @@ -435,8 +472,11 @@ void StatementSync::All(const FunctionCallbackInfo& args) { StatementSync* stmt; ASSIGN_OR_RETURN_UNWRAP(&stmt, args.This()); Environment* env = Environment::GetCurrent(args); + THROW_AND_RETURN_ON_BAD_STATE( + env, stmt->IsFinalized(), "statement has been finalized"); int r = sqlite3_reset(stmt->statement_); - CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_, r, SQLITE_OK, void()); + CHECK_ERROR_OR_THROW( + env->isolate(), stmt->db_->Connection(), r, SQLITE_OK, void()); if (!stmt->BindParams(args)) { return; @@ -462,7 +502,8 @@ void StatementSync::All(const FunctionCallbackInfo& args) { rows.emplace_back(row); } - CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_, r, SQLITE_DONE, void()); + CHECK_ERROR_OR_THROW( + env->isolate(), stmt->db_->Connection(), r, SQLITE_DONE, void()); args.GetReturnValue().Set( Array::New(env->isolate(), rows.data(), rows.size())); } @@ -471,8 +512,11 @@ void StatementSync::Get(const FunctionCallbackInfo& args) { StatementSync* stmt; ASSIGN_OR_RETURN_UNWRAP(&stmt, args.This()); Environment* env = Environment::GetCurrent(args); + THROW_AND_RETURN_ON_BAD_STATE( + env, stmt->IsFinalized(), "statement has been finalized"); int r = sqlite3_reset(stmt->statement_); - CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_, r, SQLITE_OK, void()); + CHECK_ERROR_OR_THROW( + env->isolate(), stmt->db_->Connection(), r, SQLITE_OK, void()); if (!stmt->BindParams(args)) { return; @@ -482,7 +526,7 @@ void StatementSync::Get(const FunctionCallbackInfo& args) { r = sqlite3_step(stmt->statement_); if (r == SQLITE_DONE) return; if (r != SQLITE_ROW) { - THROW_ERR_SQLITE_ERROR(env->isolate(), stmt->db_); + THROW_ERR_SQLITE_ERROR(env->isolate(), stmt->db_->Connection()); return; } @@ -511,8 +555,11 @@ void StatementSync::Run(const FunctionCallbackInfo& args) { StatementSync* stmt; ASSIGN_OR_RETURN_UNWRAP(&stmt, args.This()); Environment* env = Environment::GetCurrent(args); + THROW_AND_RETURN_ON_BAD_STATE( + env, stmt->IsFinalized(), "statement has been finalized"); int r = sqlite3_reset(stmt->statement_); - CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_, r, SQLITE_OK, void()); + CHECK_ERROR_OR_THROW( + env->isolate(), stmt->db_->Connection(), r, SQLITE_OK, void()); if (!stmt->BindParams(args)) { return; @@ -521,7 +568,7 @@ void StatementSync::Run(const FunctionCallbackInfo& args) { auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); }); r = sqlite3_step(stmt->statement_); if (r != SQLITE_ROW && r != SQLITE_DONE) { - THROW_ERR_SQLITE_ERROR(env->isolate(), stmt->db_); + THROW_ERR_SQLITE_ERROR(env->isolate(), stmt->db_->Connection()); return; } @@ -530,8 +577,9 @@ void StatementSync::Run(const FunctionCallbackInfo& args) { FIXED_ONE_BYTE_STRING(env->isolate(), "lastInsertRowid"); Local changes_string = FIXED_ONE_BYTE_STRING(env->isolate(), "changes"); - sqlite3_int64 last_insert_rowid = sqlite3_last_insert_rowid(stmt->db_); - sqlite3_int64 changes = sqlite3_changes64(stmt->db_); + sqlite3_int64 last_insert_rowid = + sqlite3_last_insert_rowid(stmt->db_->Connection()); + sqlite3_int64 changes = sqlite3_changes64(stmt->db_->Connection()); Local last_insert_rowid_val; Local changes_val; @@ -557,6 +605,8 @@ void StatementSync::SourceSQL(const FunctionCallbackInfo& args) { StatementSync* stmt; ASSIGN_OR_RETURN_UNWRAP(&stmt, args.This()); Environment* env = Environment::GetCurrent(args); + THROW_AND_RETURN_ON_BAD_STATE( + env, stmt->IsFinalized(), "statement has been finalized"); Local sql; if (!String::NewFromUtf8(env->isolate(), sqlite3_sql(stmt->statement_)) .ToLocal(&sql)) { @@ -569,6 +619,8 @@ void StatementSync::ExpandedSQL(const FunctionCallbackInfo& args) { StatementSync* stmt; ASSIGN_OR_RETURN_UNWRAP(&stmt, args.This()); Environment* env = Environment::GetCurrent(args); + THROW_AND_RETURN_ON_BAD_STATE( + env, stmt->IsFinalized(), "statement has been finalized"); char* expanded = sqlite3_expanded_sql(stmt->statement_); auto maybe_expanded = String::NewFromUtf8(env->isolate(), expanded); sqlite3_free(expanded); @@ -584,6 +636,8 @@ void StatementSync::SetAllowBareNamedParameters( StatementSync* stmt; ASSIGN_OR_RETURN_UNWRAP(&stmt, args.This()); Environment* env = Environment::GetCurrent(args); + THROW_AND_RETURN_ON_BAD_STATE( + env, stmt->IsFinalized(), "statement has been finalized"); if (!args[0]->IsBoolean()) { node::THROW_ERR_INVALID_ARG_TYPE( @@ -599,6 +653,8 @@ void StatementSync::SetReadBigInts(const FunctionCallbackInfo& args) { StatementSync* stmt; ASSIGN_OR_RETURN_UNWRAP(&stmt, args.This()); Environment* env = Environment::GetCurrent(args); + THROW_AND_RETURN_ON_BAD_STATE( + env, stmt->IsFinalized(), "statement has been finalized"); if (!args[0]->IsBoolean()) { node::THROW_ERR_INVALID_ARG_TYPE( @@ -640,7 +696,7 @@ Local StatementSync::GetConstructorTemplate( } BaseObjectPtr StatementSync::Create(Environment* env, - sqlite3* db, + DatabaseSync* db, sqlite3_stmt* stmt) { Local obj; if (!GetConstructorTemplate(env) diff --git a/src/node_sqlite.h b/src/node_sqlite.h index 56b937a719679b6..ca6e8c7f23cf40b 100644 --- a/src/node_sqlite.h +++ b/src/node_sqlite.h @@ -9,10 +9,13 @@ #include "util.h" #include +#include namespace node { namespace sqlite { +class StatementSync; + class DatabaseSync : public BaseObject { public: DatabaseSync(Environment* env, @@ -25,6 +28,10 @@ class DatabaseSync : public BaseObject { static void Close(const v8::FunctionCallbackInfo& args); static void Prepare(const v8::FunctionCallbackInfo& args); static void Exec(const v8::FunctionCallbackInfo& args); + void FinalizeStatements(); + void UntrackStatement(StatementSync* statement); + bool IsOpen(); + sqlite3* Connection(); SET_MEMORY_INFO_NAME(DatabaseSync) SET_SELF_SIZE(DatabaseSync) @@ -35,19 +42,20 @@ class DatabaseSync : public BaseObject { ~DatabaseSync() override; std::string location_; sqlite3* connection_; + std::unordered_set statements_; }; class StatementSync : public BaseObject { public: StatementSync(Environment* env, v8::Local object, - sqlite3* db, + DatabaseSync* db, sqlite3_stmt* stmt); void MemoryInfo(MemoryTracker* tracker) const override; static v8::Local GetConstructorTemplate( Environment* env); static BaseObjectPtr Create(Environment* env, - sqlite3* db, + DatabaseSync* db, sqlite3_stmt* stmt); static void All(const v8::FunctionCallbackInfo& args); static void Get(const v8::FunctionCallbackInfo& args); @@ -57,13 +65,15 @@ class StatementSync : public BaseObject { static void SetAllowBareNamedParameters( const v8::FunctionCallbackInfo& args); static void SetReadBigInts(const v8::FunctionCallbackInfo& args); + void Finalize(); + bool IsFinalized(); SET_MEMORY_INFO_NAME(StatementSync) SET_SELF_SIZE(StatementSync) private: ~StatementSync() override; - sqlite3* db_; + DatabaseSync* db_; sqlite3_stmt* statement_; bool use_big_ints_; bool allow_bare_named_params_;