Skip to content

fix: free StatementIterator resources on early loop exit - #632

Open
cpruijsen wants to merge 1 commit into
sql-js:masterfrom
cpruijsen:fix/issue-457
Open

fix: free StatementIterator resources on early loop exit#632
cpruijsen wants to merge 1 commit into
sql-js:masterfrom
cpruijsen:fix/issue-457

Conversation

@cpruijsen

@cpruijsen cpruijsen commented Sep 14, 2026

Copy link
Copy Markdown

Fixes #457.

StatementIterator frees the statement it yielded at the top of the next next() call, so a
for...of loop left by break, return or a thrown exception never frees the last one. The
prepared statement stays registered in the database's statement list and the allocated SQL buffer
stays allocated, both until close().

This implements the optional return() method in src/api.js, which the iterator protocol calls
on early termination:

StatementIterator.prototype["return"] = function () {
    if (this.activeStatement !== null) {
        this.activeStatement.free();
        this.activeStatement = null;
    }
    this.finalize();
    return { done: true };
};

Both legs matter and they free different things. finalize() releases the SQL buffer and stops the
iterator; activeStatement.free() is the one that reclaims the statement, calling
sqlite3_finalize and removing it from db.statements. It is also callable directly for code that
holds an iterator it has decided not to drain.

The property is quoted so the Closure pass does not rename it, matching how next and
getRemainingSQL are exported.

The test asserts the statement is freed, not that the iterator stopped. Those are different
claims, and only the first is this issue. Asserting next() returns { done: true } after a
break passes against a return() that calls finalize() alone and leaks the statement exactly
as before, because a finalized iterator reports done whether or not the statement was reclaimed. So
each of the three cases (break, a throwing loop body, a manual return()) keeps the statement it
was handed and asserts step() on it throws Statement closed, which is observable through the
public API and false unless free() actually ran.

I could not run emscripten here, so I checked both directions by applying the same change to the
shipped unminified dist/sql-asm-debug.js and running npm run test-asm-debug: with the full
method the suite is 24 passed, 0 failed; with the free() leg removed and finalize() left in
place, test_statement_iterator fails on Statement yielded before a break is freed. Worth a real
build on your side, since that is a patched artefact rather than a compile of this diff. The old
assertions stay, since the iterator reaching done is still worth pinning.

eslint src/api.js is clean.

A statement yielded by iterateStatements was only freed by the next
next() call, so a for...of loop exited via break, return, or a thrown
exception leaked the prepared statement and the allocated SQL buffer
until the database was closed.

Implement the optional iterator return() method so for...of reclaims
those resources automatically on early termination.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

StatementIterator leaks if not run to completion

1 participant