Skip to content

sqlite: tidy up handle ownership and lifetimes #66912

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

Open
araujogui wants to merge 4 commits into
base: main
Choose a base branch
from
+85 −92
Open

sqlite: tidy up handle ownership and lifetimes #66912

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b069411
sqlite: track statements in the Statement constructor
araujogui Oct 21, 2026
52e05f9
sqlite: use DeleteFnPtr for all SQLite handles
araujogui Oct 21, 2026
9ad233d
sqlite: settle BackupJob through a single exit point
araujogui Oct 21, 2026
b8409d9
sqlite: cache the SQLTagStore constructor template
araujogui Oct 21, 2026
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
1 change: 1 addition & 0 deletions src/env_properties.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,6 +456,7 @@
V(sqlite_statement_sync_constructor_template, v8::FunctionTemplate) \
V(sqlite_statement_sync_iterator_constructor_template, v8::FunctionTemplate) \
V(sqlite_session_constructor_template, v8::FunctionTemplate) \
V(sqlite_tag_store_constructor_template, v8::FunctionTemplate) \
V(srv_record_template, v8::DictionaryTemplate) \
V(streambaseoutputstream_constructor_template, v8::ObjectTemplate) \
V(tcp_constructor_template, v8::FunctionTemplate) \
Expand Down
144 changes: 62 additions & 82 deletions src/node_sqlite.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -645,7 +645,7 @@ class BackupJob : public ThreadPoolWork {
Local<Promise::Resolver> resolver =
Local<Promise::Resolver>::New(env()->isolate(), resolver_);
if (backup_status_ != SQLITE_OK) {
HandleBackupError(resolver);
Settle(resolver, CreateSQLiteError(isolate, dest_.get()), {});
return;
}

Expand All @@ -654,7 +654,7 @@ class BackupJob : public ThreadPoolWork {
source_->Connection(),
source_db_.c_str()));
if (!backup_) {
HandleBackupError(resolver);
Settle(resolver, CreateSQLiteError(isolate, dest_.get()), {});
return;
}

Expand All @@ -676,7 +676,7 @@ class BackupJob : public ThreadPoolWork {

if (!(backup_status_ == SQLITE_OK || backup_status_ == SQLITE_DONE ||
backup_status_ == SQLITE_BUSY || backup_status_ == SQLITE_LOCKED)) {
HandleBackupError(resolver, backup_status_);
Settle(resolver, CreateSQLiteError(isolate, backup_status_), {});
return;
}

Expand All @@ -697,17 +697,15 @@ class BackupJob : public ThreadPoolWork {
env()->remaining_pages_string(),
Integer::New(env()->isolate(), remaining_pages))
.IsNothing()) {
Settle(resolver, {}, {});
return;
}

Local<Value> argv[] = {progress_info};
TryCatch try_catch(env()->isolate());
USE(fn->Call(env()->context(), Null(env()->isolate()), 1, argv));
if (try_catch.HasCaught()) {
Local<Value> exception = try_catch.Exception();
Finalize();
resolver->Reject(env()->context(), exception).ToChecked();
delete this;
Settle(resolver, try_catch.Exception(), {});
return;
}
}
Expand All @@ -718,15 +716,11 @@ class BackupJob : public ThreadPoolWork {
}

if (backup_status_ != SQLITE_DONE) {
HandleBackupError(resolver);
Settle(resolver, CreateSQLiteError(isolate, dest_.get()), {});
return;
}

Finalize();
resolver
->Resolve(env()->context(), Integer::New(env()->isolate(), total_pages))
.ToChecked();
delete this;
Settle(resolver, {}, Integer::New(isolate, total_pages));
}

void Finalize() {
Expand All @@ -738,9 +732,7 @@ class BackupJob : public ThreadPoolWork {
}

void Cleanup() {
if (backup_) {
sqlite3_backup_finish(backup_.release());
}
backup_.reset();

if (dest_) {
backup_status_ = sqlite3_errcode(dest_.get());
Expand All @@ -749,29 +741,19 @@ class BackupJob : public ThreadPoolWork {
}

private:
void HandleBackupError(Local<Promise::Resolver> resolver) {
Local<Object> e;
if (!CreateSQLiteError(env()->isolate(), dest_.get()).ToLocal(&e)) {
Finalize();
delete this;
return;
}

// The single exit point of a backup. Rejects with `error` if it is set,
// otherwise resolves with `value` if it is set. Neither being set means an
// exception is already pending. Deletes the job.
void Settle(Local<Promise::Resolver> resolver,
MaybeLocal<Value> error,
Local<Value> value) {
Finalize();
resolver->Reject(env()->context(), e).ToChecked();
delete this;
}

void HandleBackupError(Local<Promise::Resolver> resolver, int errcode) {
Local<Object> e;
if (!CreateSQLiteError(env()->isolate(), errcode).ToLocal(&e)) {
Finalize();
delete this;
return;
Local<Value> e;
if (error.ToLocal(&e)) {
resolver->Reject(env()->context(), e).ToChecked();
} else if (!value.IsEmpty()) {
resolver->Resolve(env()->context(), value).ToChecked();
}

Finalize();
resolver->Reject(env()->context(), e).ToChecked();
delete this;
}

Expand All @@ -781,14 +763,8 @@ class BackupJob : public ThreadPoolWork {
BaseObjectPtr<Database> source_;
Global<Promise::Resolver> resolver_;
Global<Function> progressFunc_;
struct Sqlite3Deleter {
void operator()(sqlite3* db) const { sqlite3_close_v2(db); }
};
struct BackupDeleter {
void operator()(sqlite3_backup* b) const { sqlite3_backup_finish(b); }
};
std::unique_ptr<sqlite3, Sqlite3Deleter> dest_;
std::unique_ptr<sqlite3_backup, BackupDeleter> backup_;
ConnectionPtr dest_;
BackupPtr backup_;
int pages_;
int backup_status_ = SQLITE_OK;
std::string source_db_;
Expand Down Expand Up @@ -1756,6 +1732,10 @@ void Database::FinalizeStatements() {
statements_.clear();
}

void Database::TrackStatement(Statement* statement) {
statements_.insert(statement);
}

void Database::UntrackStatement(Statement* statement) {
statements_.erase(statement);
}
Expand Down Expand Up @@ -2177,6 +2157,8 @@ void Database::Close(const FunctionCallbackInfo<Value>& args) {
env, db->IsInCallback(), "database cannot be closed while in a callback");
db->FinalizeStatements();
db->DeleteSessions();
// Close through the raw handle instead of reset() so that a failed close
// keeps the handle, which the error below is read from.
int r = sqlite3_close_v2(db->connection_.get());
CHECK_ERROR_OR_THROW(env->isolate(), db, r, SQLITE_OK, void());
db->connection_.release();
Expand Down Expand Up @@ -2330,7 +2312,6 @@ void Database::Prepare(const FunctionCallbackInfo<Value>& args) {
if (!stmt) {
return;
}
db->statements_.insert(stmt.get());

if (return_arrays.has_value()) {
stmt->return_arrays_ = return_arrays.value();
Expand Down Expand Up @@ -3183,26 +3164,21 @@ void Database::CreateSession(const FunctionCallbackInfo<Value>& args) {
// getter, which may have closed the database since it was checked.
THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open");

sqlite3_session* pSession;
int r =
sqlite3session_create(db->connection_.get(), db_name.c_str(), &pSession);
sqlite3_session* raw_session;
int r = sqlite3session_create(
db->connection_.get(), db_name.c_str(), &raw_session);
CHECK_ERROR_OR_THROW(env->isolate(), db, r, SQLITE_OK, void());
bool wrapper_owns_session = false;
auto delete_session_on_failure = OnScopeLeave([&]() {
if (!wrapper_owns_session) {
sqlite3session_delete(pSession);
}
});
SessionPtr pSession(raw_session);

r = sqlite3session_attach(pSession, table == "" ? nullptr : table.c_str());
r = sqlite3session_attach(pSession.get(),
table == "" ? nullptr : table.c_str());
CHECK_ERROR_OR_THROW(env->isolate(), db, r, SQLITE_OK, void());

BaseObjectPtr<Session> session =
Session::Create(env, BaseObjectPtr<Database>(db), pSession);
Session::Create(env, BaseObjectPtr<Database>(db), std::move(pSession));
if (!session) {
return;
}
wrapper_owns_session = true;
args.GetReturnValue().Set(session->object());
}

Expand Down Expand Up @@ -3780,6 +3756,7 @@ Statement::Statement(Environment* env,
BaseObjectPtr<Database> db,
StatementPtr stmt)
: BaseObject(env, object), db_(std::move(db)), statement_(std::move(stmt)) {
db_->TrackStatement(this);
MakeWeak();
use_big_ints_ = db_->use_big_ints();
return_arrays_ = db_->return_arrays();
Expand Down Expand Up @@ -4654,24 +4631,27 @@ static inline void SetSideEffectFreeGetter(
SQLTagStore::~SQLTagStore() {}

Local<FunctionTemplate> SQLTagStore::GetConstructorTemplate(Environment* env) {
Isolate* isolate = env->isolate();
Local<FunctionTemplate> tmpl =
NewFunctionTemplate(isolate, IllegalConstructor);
tmpl->SetClassName(FIXED_ONE_BYTE_STRING(isolate, "SQLTagStore"));
tmpl->InstanceTemplate()->SetInternalFieldCount(
SQLTagStore::kInternalFieldCount);
SetProtoMethod(isolate, tmpl, "get", Get);
SetProtoMethod(isolate, tmpl, "all", All);
SetProtoMethod(isolate, tmpl, "iterate", Iterate);
SetProtoMethod(isolate, tmpl, "run", Run);
SetProtoMethod(isolate, tmpl, "clear", Clear);
SetSideEffectFreeGetter(isolate,
tmpl,
FIXED_ONE_BYTE_STRING(isolate, "capacity"),
CapacityGetter);
SetSideEffectFreeGetter(
isolate, tmpl, FIXED_ONE_BYTE_STRING(isolate, "db"), DatabaseGetter);
SetSideEffectFreeGetter(isolate, tmpl, env->size_string(), SizeGetter);
Local<FunctionTemplate> tmpl = env->sqlite_tag_store_constructor_template();
if (tmpl.IsEmpty()) {
Isolate* isolate = env->isolate();
tmpl = NewFunctionTemplate(isolate, IllegalConstructor);
tmpl->SetClassName(FIXED_ONE_BYTE_STRING(isolate, "SQLTagStore"));
tmpl->InstanceTemplate()->SetInternalFieldCount(
SQLTagStore::kInternalFieldCount);
SetProtoMethod(isolate, tmpl, "get", Get);
SetProtoMethod(isolate, tmpl, "all", All);
SetProtoMethod(isolate, tmpl, "iterate", Iterate);
SetProtoMethod(isolate, tmpl, "run", Run);
SetProtoMethod(isolate, tmpl, "clear", Clear);
SetSideEffectFreeGetter(isolate,
tmpl,
FIXED_ONE_BYTE_STRING(isolate, "capacity"),
CapacityGetter);
SetSideEffectFreeGetter(
isolate, tmpl, FIXED_ONE_BYTE_STRING(isolate, "db"), DatabaseGetter);
SetSideEffectFreeGetter(isolate, tmpl, env->size_string(), SizeGetter);
env->set_sqlite_tag_store_constructor_template(tmpl);
}
return tmpl;
}

Expand Down Expand Up @@ -4941,7 +4921,6 @@ BaseObjectPtr<Statement> SQLTagStore::PrepareStatement(
return BaseObjectPtr<Statement>();
}

session->database_->statements_.insert(stmt_obj.get());
session->sql_tags_.Put(sql, stmt_obj);
stmt = stmt_obj;
}
Expand Down Expand Up @@ -5176,9 +5155,9 @@ void StatementIterator::Return(const FunctionCallbackInfo<Value>& args) {
Session::Session(Environment* env,
Local<Object> object,
BaseObjectPtr<Database> database,
sqlite3_session* session)
SessionPtr session)
: BaseObject(env, object),
session_(std::unique_ptr<sqlite3_session, SessionDeleter>(session)),
session_(std::move(session)),
database_(std::move(database)) {
database_->sessions_.insert(this);
MakeWeak();
Expand All @@ -5190,7 +5169,7 @@ Session::~Session() {

BaseObjectPtr<Session> Session::Create(Environment* env,
BaseObjectPtr<Database> database,
sqlite3_session* session) {
SessionPtr session) {
Local<Object> obj;
if (!GetConstructorTemplate(env)
->InstanceTemplate()
Expand All @@ -5199,7 +5178,8 @@ BaseObjectPtr<Session> Session::Create(Environment* env,
return nullptr;
}

return MakeBaseObject<Session>(env, obj, std::move(database), session);
return MakeBaseObject<Session>(
env, obj, std::move(database), std::move(session));
}

Local<FunctionTemplate> Session::GetConstructorTemplate(Environment* env) {
Expand Down Expand Up @@ -5248,7 +5228,7 @@ void Session::Changeset(const FunctionCallbackInfo<Value>& args) {
CHECK_ERROR_OR_THROW(
env->isolate(), session->database_.get(), r, SQLITE_OK, void());

auto freeChangeset = OnScopeLeave([&] { sqlite3_free(pChangeset); });
DeleteFnPtr<void, sqlite3_free> changeset(pChangeset);

Local<ArrayBuffer> buffer = ArrayBuffer::New(env->isolate(), nChangeset);
if (nChangeset > 0) {
Expand Down
26 changes: 16 additions & 10 deletions src/node_sqlite.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,18 @@ inline void FinalizeStatement(sqlite3_stmt* stmt) {
sqlite3_finalize(stmt);
}

inline void CloseConnection(sqlite3* db) {
sqlite3_close_v2(db);
}

inline void FinishBackup(sqlite3_backup* backup) {
sqlite3_backup_finish(backup);
}

using StatementPtr = DeleteFnPtr<sqlite3_stmt, FinalizeStatement>;
using ConnectionPtr = DeleteFnPtr<sqlite3, CloseConnection>;
using BackupPtr = DeleteFnPtr<sqlite3_backup, FinishBackup>;
using SessionPtr = DeleteFnPtr<sqlite3_session, sqlite3session_delete>;

class StatementExecutionHelper {
public:
Expand Down Expand Up @@ -262,6 +273,7 @@ class Database : public BaseObject {
void RemoveBackup(BackupJob* backup);
void AddBackup(BackupJob* backup);
void FinalizeBackups();
void TrackStatement(Statement* statement);
void UntrackStatement(Statement* statement);
bool IsOpen();
// SQL functions are one of several paths by which SQLite can invoke JS.
Expand Down Expand Up @@ -337,10 +349,7 @@ class Database : public BaseObject {
DatabaseOpenConfiguration open_config_;
bool allow_load_extension_;
bool enable_load_extension_;
struct ConnectionDeleter {
void operator()(sqlite3* db) const { sqlite3_close_v2(db); }
};
std::unique_ptr<sqlite3, ConnectionDeleter> connection_;
ConnectionPtr connection_;
bool ignore_next_sqlite_error_;
int callback_depth_ = 0;
int destructor_depth_ = 0;
Expand Down Expand Up @@ -462,7 +471,7 @@ class Session : public BaseObject {
Session(Environment* env,
v8::Local<v8::Object> object,
BaseObjectPtr<Database> database,
sqlite3_session* session);
SessionPtr session);
~Session() override;
template <Sqlite3ChangesetGenFunc sqliteChangesetFunc>
static void Changeset(const v8::FunctionCallbackInfo<v8::Value>& args);
Expand All @@ -472,18 +481,15 @@ class Session : public BaseObject {
Environment* env);
static BaseObjectPtr<Session> Create(Environment* env,
BaseObjectPtr<Database> database,
sqlite3_session* session);
SessionPtr session);

void MemoryInfo(MemoryTracker* tracker) const override;
SET_MEMORY_INFO_NAME(Session)
SET_SELF_SIZE(Session)

private:
void Delete();
struct SessionDeleter {
void operator()(sqlite3_session* s) const { sqlite3session_delete(s); }
};
std::unique_ptr<sqlite3_session, SessionDeleter> session_;
SessionPtr session_;
BaseObjectPtr<Database> database_; // The Parent Database
bool is_generating_changeset_ = false;

Expand Down
6 changes: 6 additions & 0 deletions test/parallel/test-sqlite-template-tag.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,12 @@ test('sql.db returns the associated Database instance', () => {
assert.strictEqual(sql.db, db);
});

test('tag stores share a prototype', () => {
const other = new Database(':memory:').createTagStore();
assert.strictEqual(Object.getPrototypeOf(other), Object.getPrototypeOf(sql));
assert.strictEqual(other.constructor, sql.constructor);
});

test('sql error messages are descriptive', () => {
assert.strictEqual(sql.run`INSERT INTO foo (text) VALUES (${'test'})`.changes, 1);

Expand Down
Loading