Skip to content

sqlite: should Database close its connection on garbage collection? #67427

Open

Description

@TrevorBurnham

Background

If a Database is garbage-collected while still open, its destructor (Database::~Database() in src/node_sqlite.cc) finalizes outstanding statements, deletes sessions, and closes the connection without notice.

fs.FileHandle used to behave the same way and moved away from it. Closing on GC was runtime-deprecated in v14 and became End-of-Life (it throws) in v25 (DEP0137). @BurningEnlightenment raised this in #57445 as something to settle before node:sqlite is stable, because tightening the behavior afterward would be semver-major.

Why it might matter for SQLite

  • Until it's collected, an unclosed connection keeps its file locks, and in WAL mode it may delay the checkpoint. That timing depends on GC, so other connections or processes can hit SQLITE_BUSY at unpredictable times.
  • A connection closed implicitly can hide bugs, such as a forgotten close() in a transaction path.

Prior art

  • better-sqlite3 does what we do today: its destructor finalizes statements and closes the connection without notice, and it closes any remaining connections when the addon unloads. Matching it eases migration, which is a frequent request (e.g. sqlite: bind parameters the same way better-sqlite3 does. #67353).
  • SQLite's official WASM build (@sqlite.org/sqlite-wasm) never closes on GC. Its docs say GC "will never trigger close()", so an unclosed connection leaks.
  • fs.FileHandle went from closing on GC to throwing (DEP0137). Part of the reason was async operations racing a GC-driven close, which doesn't apply here: all node:sqlite operations are synchronous, so nothing can be in flight when the destructor runs.
  • using / Symbol.dispose already provide explicit cleanup.

Options

  1. Keep the current behavior and document it as guaranteed.
  2. Keep closing on GC, but emit a warning when it happens. This could be added later without being semver-major.
  3. Follow DEP0137: deprecate, then throw.

Which behavior do we want to commit to for the stable API?

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions