diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 8978670d07e..9a7769c8159 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -1109,7 +1109,7 @@ bool VirtualTableModule::CanCallIntoJS() const { bool VirtualTableModule::CloseIterator(NodeVTabCursor* cursor) { VirtualTableModule* mod = cursor->module; - // Skipped in two cases: + // Skipped in three cases: // // - While the database is being torn down from a destructor, because those // run from a garbage collection callback where JavaScript cannot be @@ -1120,7 +1120,11 @@ bool VirtualTableModule::CloseIterator(NodeVTabCursor* cursor) { // 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() || + // - When next() has already reported `done: true`, because the iteration ran + // to completion. `return()` is only called on abrupt termination in + // `for...of`; a finished iterator must not be asked to clean up again, and + // a custom iterable may throw from `return()` even after completion. + if (cursor->iterator.IsEmpty() || cursor->done || !mod->CanCallIntoJS() || mod->env_->isolate()->HasPendingException()) { return false; } diff --git a/test/parallel/test-sqlite-virtual-table.js b/test/parallel/test-sqlite-virtual-table.js index c10b343b12b..be41ed73fa0 100644 --- a/test/parallel/test-sqlite-virtual-table.js +++ b/test/parallel/test-sqlite-virtual-table.js @@ -622,6 +622,41 @@ suite('Database.prototype.createModule()', () => { assert.deepStrictEqual(cleanedUp, [1, 2]); }); + test('does not call return() on a normally exhausted iterator', () => { + // Once next() reports `done: true` the iterator is finished, so no + // cleanup should run. A throwing return() must not fail an otherwise + // successful query. Matches `for...of`, which only calls return() on + // abrupt termination. + const db = new Database(':memory:'); + for (const [label, makeRows] of [ + ['empty', () => { + return { + [Symbol.iterator]() { return this; }, + next() { return { value: [], done: true }; }, + return() { throw new Error('cleanup boom'); }, + }; + }], + ['fully_consumed', () => { + let i = 0; + return { + [Symbol.iterator]() { return this; }, + next() { return { value: [i++], done: i > 2 }; }, + return() { throw new Error('cleanup boom'); }, + }; + }], + ]) { + db.createModule(`exhausted_${label}`, { + columns: [{ name: 'v', type: 'INTEGER' }], + rows: makeRows, + }); + + const rows = db.prepare(`SELECT v FROM exhausted_${label}`).all(); + assert.deepStrictEqual( + rows.map((row) => row.v), + label === 'empty' ? [] : [0, 1]); + } + }); + 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