diff --git a/src/env_properties.h b/src/env_properties.h index 087193faba98..2a9766877974 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -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) \ diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index b4fbfa88746a..650df77e6505 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -645,7 +645,7 @@ class BackupJob : public ThreadPoolWork { Local resolver = Local::New(env()->isolate(), resolver_); if (backup_status_ != SQLITE_OK) { - HandleBackupError(resolver); + Settle(resolver, CreateSQLiteError(isolate, dest_.get()), {}); return; } @@ -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; } @@ -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; } @@ -697,6 +697,7 @@ class BackupJob : public ThreadPoolWork { env()->remaining_pages_string(), Integer::New(env()->isolate(), remaining_pages)) .IsNothing()) { + Settle(resolver, {}, {}); return; } @@ -704,10 +705,7 @@ class BackupJob : public ThreadPoolWork { TryCatch try_catch(env()->isolate()); USE(fn->Call(env()->context(), Null(env()->isolate()), 1, argv)); if (try_catch.HasCaught()) { - Local exception = try_catch.Exception(); - Finalize(); - resolver->Reject(env()->context(), exception).ToChecked(); - delete this; + Settle(resolver, try_catch.Exception(), {}); return; } } @@ -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() { @@ -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()); @@ -749,29 +741,19 @@ class BackupJob : public ThreadPoolWork { } private: - void HandleBackupError(Local resolver) { - Local 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 resolver, + MaybeLocal error, + Local value) { Finalize(); - resolver->Reject(env()->context(), e).ToChecked(); - delete this; - } - - void HandleBackupError(Local resolver, int errcode) { - Local e; - if (!CreateSQLiteError(env()->isolate(), errcode).ToLocal(&e)) { - Finalize(); - delete this; - return; + Local 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; } @@ -781,14 +763,8 @@ class BackupJob : public ThreadPoolWork { BaseObjectPtr source_; Global resolver_; Global 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 dest_; - std::unique_ptr backup_; + ConnectionPtr dest_; + BackupPtr backup_; int pages_; int backup_status_ = SQLITE_OK; std::string source_db_; @@ -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); } @@ -2177,6 +2157,8 @@ void Database::Close(const FunctionCallbackInfo& 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(); @@ -2330,7 +2312,6 @@ void Database::Prepare(const FunctionCallbackInfo& args) { if (!stmt) { return; } - db->statements_.insert(stmt.get()); if (return_arrays.has_value()) { stmt->return_arrays_ = return_arrays.value(); @@ -3183,26 +3164,21 @@ void Database::CreateSession(const FunctionCallbackInfo& 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::Create(env, BaseObjectPtr(db), pSession); + Session::Create(env, BaseObjectPtr(db), std::move(pSession)); if (!session) { return; } - wrapper_owns_session = true; args.GetReturnValue().Set(session->object()); } @@ -3780,6 +3756,7 @@ Statement::Statement(Environment* env, BaseObjectPtr 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(); @@ -4654,24 +4631,27 @@ static inline void SetSideEffectFreeGetter( SQLTagStore::~SQLTagStore() {} Local SQLTagStore::GetConstructorTemplate(Environment* env) { - Isolate* isolate = env->isolate(); - Local 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 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; } @@ -4941,7 +4921,6 @@ BaseObjectPtr SQLTagStore::PrepareStatement( return BaseObjectPtr(); } - session->database_->statements_.insert(stmt_obj.get()); session->sql_tags_.Put(sql, stmt_obj); stmt = stmt_obj; } @@ -5176,9 +5155,9 @@ void StatementIterator::Return(const FunctionCallbackInfo& args) { Session::Session(Environment* env, Local object, BaseObjectPtr database, - sqlite3_session* session) + SessionPtr session) : BaseObject(env, object), - session_(std::unique_ptr(session)), + session_(std::move(session)), database_(std::move(database)) { database_->sessions_.insert(this); MakeWeak(); @@ -5190,7 +5169,7 @@ Session::~Session() { BaseObjectPtr Session::Create(Environment* env, BaseObjectPtr database, - sqlite3_session* session) { + SessionPtr session) { Local obj; if (!GetConstructorTemplate(env) ->InstanceTemplate() @@ -5199,7 +5178,8 @@ BaseObjectPtr Session::Create(Environment* env, return nullptr; } - return MakeBaseObject(env, obj, std::move(database), session); + return MakeBaseObject( + env, obj, std::move(database), std::move(session)); } Local Session::GetConstructorTemplate(Environment* env) { @@ -5248,7 +5228,7 @@ void Session::Changeset(const FunctionCallbackInfo& args) { CHECK_ERROR_OR_THROW( env->isolate(), session->database_.get(), r, SQLITE_OK, void()); - auto freeChangeset = OnScopeLeave([&] { sqlite3_free(pChangeset); }); + DeleteFnPtr changeset(pChangeset); Local buffer = ArrayBuffer::New(env->isolate(), nChangeset); if (nChangeset > 0) { diff --git a/src/node_sqlite.h b/src/node_sqlite.h index 784f957acaf7..64998658a9b5 100644 --- a/src/node_sqlite.h +++ b/src/node_sqlite.h @@ -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; +using ConnectionPtr = DeleteFnPtr; +using BackupPtr = DeleteFnPtr; +using SessionPtr = DeleteFnPtr; class StatementExecutionHelper { public: @@ -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. @@ -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 connection_; + ConnectionPtr connection_; bool ignore_next_sqlite_error_; int callback_depth_ = 0; int destructor_depth_ = 0; @@ -462,7 +471,7 @@ class Session : public BaseObject { Session(Environment* env, v8::Local object, BaseObjectPtr database, - sqlite3_session* session); + SessionPtr session); ~Session() override; template static void Changeset(const v8::FunctionCallbackInfo& args); @@ -472,7 +481,7 @@ class Session : public BaseObject { Environment* env); static BaseObjectPtr Create(Environment* env, BaseObjectPtr database, - sqlite3_session* session); + SessionPtr session); void MemoryInfo(MemoryTracker* tracker) const override; SET_MEMORY_INFO_NAME(Session) @@ -480,10 +489,7 @@ class Session : public BaseObject { private: void Delete(); - struct SessionDeleter { - void operator()(sqlite3_session* s) const { sqlite3session_delete(s); } - }; - std::unique_ptr session_; + SessionPtr session_; BaseObjectPtr database_; // The Parent Database bool is_generating_changeset_ = false; diff --git a/test/parallel/test-sqlite-template-tag.js b/test/parallel/test-sqlite-template-tag.js index f7a5053a3301..5ecf055523f4 100644 --- a/test/parallel/test-sqlite-template-tag.js +++ b/test/parallel/test-sqlite-template-tag.js @@ -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);