Release statements and iterators when the database is closed - #236
Merged
Merged
Conversation
Statements and row iterators hold their own references to the libSQL connection, and libSQL only closes the SQLite handle when the last reference is dropped. Database.close() therefore left the database file open (and any read transaction of a partially consumed iterator active) until the garbage collector reclaimed every statement. On Windows this makes deleting the database file after close() fail with EBUSY. Track each database's statements and iterators with weak references and release their libSQL objects on close(). Using a statement or iterator afterwards throws "The database connection is not open", matching better-sqlite3.
A local libSQL row references the statement it was read from, and through it the connection. The Record returned by RowsIterator.next() kept that row alive until the garbage collector reclaimed it, so the database file stayed open after Database.close(), failing file deletion with EBUSY on Windows. Read the column values when the row is produced so records no longer reference the statement.
Contributor
Author
|
@claude review |
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
1 participant
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.
Statements and row iterators hold their own references to the libSQL connection, and libSQL only closes the SQLite handle when the last reference is dropped.
Database.close()therefore left the database file open (and any read transaction of a partially consumed iterator active) until the garbage collector reclaimed every statement. On Windows this makes deleting the database file afterclose()fail withEBUSY, which is what breaks Windows CI on #233.This tracks each database's statements and iterators with weak references and releases their libSQL objects on
close(). In-flight operations hold their own references and finish normally; an iterator closed mid-next()drops its rows once that call completes. Using a statement or iterator afterclose()throwsTypeError: The database connection is not open, matching better-sqlite3. The iterator wrappers incompat.jsandpromise.jsnow run errors throughconvertErrorso that surfaces as the sameTypeError.A second reference was hiding in iterator results: a local libSQL
Rowholds a clone of the statement (and so the connection), and theRecordreturned byRowsIterator.next()kept that row alive until GC. Records now copy the column values out when the row is produced (only the first column when plucking), so they no longer pin the connection.Unlike better-sqlite3, closing with an active iterator is still allowed (better-sqlite3 throws "This database connection is busy executing a query"), so the active-iterator sync test is libsql-only.
Tests: new regression tests in
sync.test.jsandasync.test.jsfail withSQLITE_BUSYwithout the fix and pass with it.