From cefe77fdaaa3b9658feaed266c7eba5eb4ec7614 Mon Sep 17 00:00:00 2001 From: "Kamat, Trivikram" <16024985+trivikr@users.noreply.github.com> Date: Mon, 21 Sep 2026 22:46:35 -0700 Subject: [PATCH] sqlite: run generator return() on cursor refilter SQLite re-invokes xFilter on a cursor it already used, as it does for the inner table of a correlated subquery or join, abandoning the previous iterator mid-loop. Call its return() method so generator `finally` blocks still run. Refactor the xClose cleanup intoCloseIterator() so both paths share it; on refilter, a throwing cleanup is surfaced as the query error. Signed-off-by: Kamat, Trivikram <16024985+trivikr@users.noreply.github.com> Assisted-by: opencode --- src/node_sqlite.cc | 94 +++++++++++++--------- src/node_sqlite.h | 5 ++ test/parallel/test-sqlite-virtual-table.js | 30 +++++++ 3 files changed, 91 insertions(+), 38 deletions(-) diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 56babd7adaec..0e9691a13b2a 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -1097,6 +1097,50 @@ bool VirtualTableModule::CanCallIntoJS() const { return db_ && !db_->IsInDestructor(); } +bool VirtualTableModule::CloseIterator(NodeVTabCursor* cursor) { + VirtualTableModule* mod = cursor->module; + + // Skipped in two cases: + // + // - While the database is being torn down from a destructor, because those + // run from a garbage collection callback where JavaScript cannot be + // executed. An abandoned generator does not run `finally` in JavaScript + // either, so skipping matches the language. + // - When an error is already pending, because calling into JavaScript would + // discard it and the caller would see an empty result instead of the error. + // A generator whose own body threw has already run its `finally` as part of + // that throw, so this only affects an iterator abandoned while suspended + // because something else failed. + if (cursor->iterator.IsEmpty() || !mod->CanCallIntoJS() || + mod->env_->isolate()->HasPendingException()) { + return false; + } + + Environment* env = mod->env_; + Isolate* isolate = env->isolate(); + HandleScope handle_scope(isolate); + CallbackDepthGuard callback_guard(mod->db_.get()); + + // Scoped above the property lookup so a throwing `return` getter is + // handled the same way as a throwing `return()` method. + TryCatch try_catch(isolate); + Local iterator = cursor->iterator.Get(isolate); + Local return_method; + if (iterator->Get(env->context(), FIXED_ONE_BYTE_STRING(isolate, "return")) + .ToLocal(&return_method) && + return_method->IsFunction()) { + USE(return_method.As()->Call( + env->context(), iterator, 0, nullptr)); + } + + // Re-throw so that a throwing `finally` is not silently discarded. + if (try_catch.HasCaught() && !try_catch.HasTerminated()) { + try_catch.ReThrow(); + return true; + } + return false; +} + void VirtualTableModule::ReleaseHiddenValues(NodeVTabCursor* cursor) { for (sqlite3_value*& value : cursor->hidden_values) { if (value != nullptr) { @@ -1216,44 +1260,9 @@ int VirtualTableModule::xClose(sqlite3_vtab_cursor* pCursor) { // Close the iterator so generator `finally` blocks still run when SQLite // stops stepping early, as it does for LIMIT or a `break` out of a for...of - // loop. Skipped in two cases: - // - // - While the database is being torn down from a destructor, because those - // run from a garbage collection callback where JavaScript cannot be - // executed. An abandoned generator does not run `finally` in JavaScript - // either, so skipping matches the language. - // - When an error is already pending, because calling into JavaScript would - // discard it and the caller would see an empty result instead of the error. - // A generator whose own body threw has already run its `finally` as part of - // that throw, so this only affects an iterator abandoned while suspended - // because something else failed. - if (!cursor->iterator.IsEmpty() && mod->CanCallIntoJS() && - !mod->env_->isolate()->HasPendingException()) { - Environment* env = mod->env_; - Isolate* isolate = env->isolate(); - HandleScope handle_scope(isolate); - CallbackDepthGuard callback_guard(mod->db_.get()); - - // Scoped above the property lookup so a throwing `return` getter is - // handled the same way as a throwing `return()` method. - TryCatch try_catch(isolate); - Local iterator = cursor->iterator.Get(isolate); - Local return_method; - if (iterator->Get(env->context(), FIXED_ONE_BYTE_STRING(isolate, "return")) - .ToLocal(&return_method) && - return_method->IsFunction()) { - USE(return_method.As()->Call( - env->context(), iterator, 0, nullptr)); - } - - // Re-throw so that a throwing `finally` is not silently discarded. SQLite - // discards xClose's return value, so there is no SQLite error here to - // suppress; calling PropagateJSError would leave the suppression flag set - // and swallow the next unrelated SQLite error. - if (try_catch.HasCaught() && !try_catch.HasTerminated()) { - try_catch.ReThrow(); - } - } + // loop. SQLite discards xClose's return value, so a throwing cleanup is + // re-thrown rather than paired with a SQLite error here. + mod->CloseIterator(cursor); ReleaseHiddenValues(cursor); cursor->iterator.Reset(); @@ -1277,6 +1286,15 @@ int VirtualTableModule::xFilter(sqlite3_vtab_cursor* pCursor, } CallbackDepthGuard callback_guard(mod->db_.get()); + // Re-filtering a cursor occurs when SQLite re-invokes xFilter on a cursor it + // already used, as it does for the inner table of a correlated subquery or + // join. The previous iterator is abandoned mid-loop, so close it the same way + // xClose does; otherwise its generator `finally` blocks never run. A throwing + // cleanup is surfaced as the error for this query. + if (mod->CloseIterator(cursor)) { + return mod->PropagateJSError(); + } + cursor->rowid = 0; cursor->done = false; cursor->iterator.Reset(); diff --git a/src/node_sqlite.h b/src/node_sqlite.h index 81d772556722..27a8034b7a77 100644 --- a/src/node_sqlite.h +++ b/src/node_sqlite.h @@ -717,6 +717,11 @@ class VirtualTableModule { // from a garbage collection callback where JavaScript cannot be executed. bool CanCallIntoJS() const; + // Runs the iterator's return() method so generator `finally` blocks still run + // when an iterator is abandoned while suspended. Returns true if the return() + // threw; the exception is re-thrown for the caller to surface. + bool CloseIterator(NodeVTabCursor* cursor); + static void ReleaseHiddenValues(NodeVTabCursor* cursor); Environment* env_; diff --git a/test/parallel/test-sqlite-virtual-table.js b/test/parallel/test-sqlite-virtual-table.js index 89d6252bbe43..c7fffda2f381 100644 --- a/test/parallel/test-sqlite-virtual-table.js +++ b/test/parallel/test-sqlite-virtual-table.js @@ -558,6 +558,36 @@ suite('DatabaseSync.prototype.createModule()', () => { } }); + test('closes the iterator when a filter is reapplied to the cursor', () => { + // A correlated subquery re-invokes xFilter on the same cursor per outer + // row, abandoning the previous iterator; its `finally` must still run. + const db = new DatabaseSync(':memory:'); + const cleanedUp = []; + + db.createModule('refilter_cleanup', { + columns: [ + { name: 'input', type: 'INTEGER', hidden: true }, + ], + *rows(input) { + try { + yield [input]; + } finally { + cleanedUp.push(input); + } + }, + }); + + db.exec('CREATE TABLE t(a); INSERT INTO t VALUES (1), (2)'); + const rows = db.prepare( + 'SELECT a FROM t WHERE EXISTS(SELECT 1 FROM refilter_cleanup(t.a))' + ).all(); + assert.deepStrictEqual(rows, [ + { __proto__: null, a: 1 }, + { __proto__: null, a: 2 }, + ]); + assert.deepStrictEqual(cleanedUp, [1, 2]); + }); + test('does not run cleanup when the statement is collected', () => { // The destructor runs from a GC callback, where JavaScript cannot be // executed. An abandoned generator does not run `finally` in JavaScript