Skip to content

sqlite: add StatementSync.prototype.close() and [Symbol.dispose]()#64232

Open
araujogui wants to merge 4 commits into
nodejs:mainfrom
araujogui:sqlite-statement-sync-dispose
Open

sqlite: add StatementSync.prototype.close() and [Symbol.dispose]()#64232
araujogui wants to merge 4 commits into
nodejs:mainfrom
araujogui:sqlite-statement-sync-dispose

Conversation

@araujogui

Copy link
Copy Markdown
Member

No description provided.

Copilot AI review requested due to automatic review settings July 1, 2026 14:22
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/sqlite

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem. labels Jul 1, 2026

This comment was marked as low quality.

@aduh95 aduh95 added semver-minor PRs that contain new features and should be released in the next minor version. backport-requested-v24.x PRs awaiting manual backport to the v24.x-staging branch. labels Jul 2, 2026
@Renegade334

Copy link
Copy Markdown
Member

We are currently finalizing statements when the wrapper is deconstructed after GC. This PR only makes sense if we are going to change this to an explicit "user should finalize statements themselves" model, in which case we should also expose a named .finalize() method that does the same thing. If not, then this probably isn't a logical addition.

@araujogui

araujogui commented Jul 3, 2026

Copy link
Copy Markdown
Member Author

We are currently finalizing statements when the wrapper is deconstructed after GC. This PR only makes sense if we are going to change this to an explicit "user should finalize statements themselves" model, in which case we should also expose a named .finalize() method that does the same thing. If not, then this probably isn't a logical addition.

I think we can support both approaches. I don't see any issue with that.

We can either let the GC finalize statements as they are today, or let users finalize them explicitly with a dispose() or the using keyword. The choice is theirs. Explicit finalization is very useful in memory-constrained environments.

@araujogui araujogui changed the title sqlite: add StatementSync.prototype[Symbol.dispose]() sqlite: add StatementSync.prototype.close() and [Symbol.dispose]() Jul 4, 2026
Comment thread src/node_sqlite.cc Outdated
@jasnell

jasnell commented Jul 4, 2026

Copy link
Copy Markdown
Member

... We are currently finalizing statements when the wrapper is deconstructed after GC. This PR only makes sense if we are going to change this to an explicit "user should finalize statements themselves" model ...

Honestly, I'm not a fan at all of relying on GC timing. It's too unpredictable and can expose GC timing details (which can be problematic on its own). We originally had FileHandle automatically close on GC but have since backed away from that to require explicit closing. I think it makes sense to do the same here.

@Renegade334

Copy link
Copy Markdown
Member

That's the case I had in mind. I don't have a problem with the approach, we just need to be very clear to consumers that the contract is changing.

Comment thread src/node_sqlite.cc Outdated
@araujogui araujogui force-pushed the sqlite-statement-sync-dispose branch from cb5696c to 9ed6fae Compare July 4, 2026 15:58
araujogui added 4 commits July 8, 2026 13:26
This extends explicit resource management support to prepared
statements, allowing a StatementSync to be deterministically
finalized via a `using` declaration, mirroring the existing
DatabaseSync and Session dispose methods.

Signed-off-by: Guilherme Araújo <arauujogui@gmail.com>
Signed-off-by: Guilherme Araújo <arauujogui@gmail.com>
Signed-off-by: Guilherme Araújo <arauujogui@gmail.com>
Signed-off-by: Guilherme Araújo <arauujogui@gmail.com>
@araujogui araujogui force-pushed the sqlite-statement-sync-dispose branch from 9ed6fae to 7144743 Compare July 8, 2026 16:29

@Renegade334 Renegade334 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. As a follow-up, we should update all of the statement examples in sqlite.md to demonstrate explicit teardown.

@codecov

codecov Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.50000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.24%. Comparing base (799e910) to head (7144743).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/node_sqlite.cc 87.50% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #64232      +/-   ##
==========================================
- Coverage   90.25%   90.24%   -0.02%     
==========================================
  Files         741      741              
  Lines      241165   241177      +12     
  Branches    45428    45426       -2     
==========================================
- Hits       217667   217643      -24     
- Misses      15069    15114      +45     
+ Partials     8429     8420       -9     
Files with missing lines Coverage Δ
src/node_sqlite.h 80.64% <ø> (ø)
src/node_sqlite.cc 81.06% <87.50%> (+0.34%) ⬆️

... and 34 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

Labels

backport-requested-v24.x PRs awaiting manual backport to the v24.x-staging branch. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version. sqlite Issues and PRs related to the SQLite subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants