fix: free StatementIterator resources on early loop exit - #632
Open
cpruijsen wants to merge 1 commit into
Open
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #457.
StatementIteratorfrees the statement it yielded at the top of the nextnext()call, so afor...ofloop left bybreak,returnor a thrown exception never frees the last one. Theprepared 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 insrc/api.js, which the iterator protocol callson early termination:
Both legs matter and they free different things.
finalize()releases the SQL buffer and stops theiterator;
activeStatement.free()is the one that reclaims the statement, callingsqlite3_finalizeand removing it fromdb.statements. It is also callable directly for code thatholds an iterator it has decided not to drain.
The property is quoted so the Closure pass does not rename it, matching how
nextandgetRemainingSQLare 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 abreakpasses against areturn()that callsfinalize()alone and leaks the statement exactlyas 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 manualreturn()) keeps the statement itwas handed and asserts
step()on it throwsStatement closed, which is observable through thepublic 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.jsand runningnpm run test-asm-debug: with the fullmethod the suite is 24 passed, 0 failed; with the
free()leg removed andfinalize()left inplace,
test_statement_iteratorfails onStatement yielded before a break is freed. Worth a realbuild on your side, since that is a patched artefact rather than a compile of this diff. The old
assertions stay, since the iterator reaching
doneis still worth pinning.eslint src/api.jsis clean.