Commit 8bf65d4beb4 for nodejs
commit 8bf65d4beb45b871e01210d59153173f33d45891
Author: Trivikram Kamat <trivikr.dev@gmail.com>
Date: Mon Sep 21 22:46:35 2026 -0700
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
PR-URL: https://github.com/nodejs/node/pull/66195
Fixes: https://github.com/nodejs/node/issues/66193
Reviewed-By: Guilherme Araújo <arauujogui@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc
index 56babd7adae..0e9691a13b2 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<Object> iterator = cursor->iterator.Get(isolate);
+ Local<Value> return_method;
+ if (iterator->Get(env->context(), FIXED_ONE_BYTE_STRING(isolate, "return"))
+ .ToLocal(&return_method) &&
+ return_method->IsFunction()) {
+ USE(return_method.As<Function>()->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<Object> iterator = cursor->iterator.Get(isolate);
- Local<Value> return_method;
- if (iterator->Get(env->context(), FIXED_ONE_BYTE_STRING(isolate, "return"))
- .ToLocal(&return_method) &&
- return_method->IsFunction()) {
- USE(return_method.As<Function>()->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 81d77255672..27a8034b7a7 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 89d6252bbe4..c7fffda2f38 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