Repository: denoland/deno
Pull request: fix(ext/node): handle errors in sqlite session filter callback and ad…

Closes https://github.com/denoland/deno/issues/31635

Fixes two issues in the `node:sqlite` module:

 1. The filter handler in `applyChangeset` was panicking when the JavaScript filter callback threw an exception. The code used `.unwrap()` on the result of `filter.call()`, which returns `None` when an exception is thrown. Added proper exception handling with `tc_scope` and a `has_caught` flag to preserve the first exception.

 2. The Session class was missing `Symbol.dispose` support, causing the "session supports ERM" test to fail when using the `using` keyword. Added `[Symbol.dispose]` to Session.prototype.

Changed files:
filename: ext/node/ops/sqlite/database.rs
patch: @@ -963,6 +963,7 @@ impl DatabaseSync {
       scope: &'a mut v8::PinScope<'b, 'c>,
       confict: Option<v8::Local<'b, v8::Function>>,
       filter: Option<v8::Local<'b, v8::Function>>,
+      has_caught: bool,
     }
 
     // Conflict handler callback for `sqlite3changeset_apply()`.
@@ -1014,14 +1015,30 @@ impl DatabaseSync {
       unsafe {
         let ctx = &mut *(p_ctx as *mut HandlerCtx);
 
+        // If we've already caught an exception, don't call the filter again
+        // to avoid overwriting the original exception.
+        if ctx.has_caught {
+          return 0;
+        }
+
         if let Some(filter) = &mut ctx.filter {
           let tab = CStr::from_ptr(z_tab).to_str().unwrap();
 
           let recv = v8::undefined(ctx.scope).into();
           let args = [v8::String::new(ctx.scope, tab).unwrap().into()];
 
-          let ret = filter.call(ctx.scope, recv, &args).unwrap();
-          return ret.boolean_value(ctx.scope) as i32;
+          v8::tc_scope!(tc_scope, ctx.scope);
+
+          let ret = filter
+            .call(tc_scope, recv, &args)
+            .unwrap_or_else(|| v8::undefined(tc_scope).into());
+          if tc_scope.has_caught() {
+            ctx.has_caught = true;
+            tc_scope.rethrow();
+            return 0;
+          }
+
+          return ret.boolean_value(tc_scope) as i32;
         }
 
         1
@@ -1037,6 +1054,7 @@ impl DatabaseSync {
       scope,
       confict: None,
       filter: None,
+      has_caught: false,
     };
 
     if let Some(options) = options {

filename: ext/node/polyfills/sqlite.ts
patch: @@ -4,6 +4,7 @@ import { primordials } from "ext:core/mod.js";
 import {
   DatabaseSync as DatabaseSyncOp,
   op_node_database_backup,
+  Session,
   StatementSync,
 } from "ext:core/ops";
 import type { Buffer } from "node:buffer";
@@ -214,6 +215,22 @@ ObjectDefineProperties(DatabaseSync.prototype, {
   },
 });
 
+ObjectDefineProperties(Session.prototype, {
+  [SymbolDispose]: {
+    __proto__: null,
+    value: function () {
+      try {
+        this.close();
+      } catch {
+        // Ignore errors.
+      }
+    },
+    enumerable: true,
+    configurable: true,
+    writable: true,
+  },
+});
+
 export { backup, DatabaseSync, StatementSync };
 
 export default {

filename: tests/node_compat/config.toml
patch: @@ -987,7 +987,7 @@
 "parallel/test-sqlite-database-sync-dispose.js" = {}
 "parallel/test-sqlite-database-sync.js" = { darwin = false, linux = false, windows = false, reason = "https://github.com/denoland/deno/issues/31634" }
 "parallel/test-sqlite-named-parameters.js" = {}
-"parallel/test-sqlite-session.js" = { darwin = false, linux = false, windows = false, reason = "https://github.com/denoland/deno/issues/31635" }
+"parallel/test-sqlite-session.js" = {}
 "parallel/test-sqlite-statement-sync-columns.js" = {}
 "parallel/test-sqlite-timeout.js" = {}
 "parallel/test-sqlite-transactions.js" = {}