Use native finalizers for db and stmts

Author: simolus3

sqlite3/lib/src/ffi/bindings.dart

@@ -812,18 +812,65 @@ final class FfiChangesetIterator implements RawChangesetIterator, Finalizable {
   }
 }
 
-final class FfiDatabase implements RawSqliteDatabase {
+/// For user-defined functions, SQLite hooks, or virtual file systems, we
+/// register function pointers as [NativeCallable]s.
+///
+/// To avoid leaking resources, we should [NativeCallable.close] those once
+/// they're no longer used. SQLite provides the `xDestroy` callback for this.
+/// For a long time, this package used an additional callable for `xDestroy`
+/// that closed the original callables and itself.
+///
+/// After migrating to native finalizers however, this approach stopped working.
+/// Because native finalizers can run as the Dart isolate is shutting down, we
+/// can't invoke Dart code anymore. There is no good way to close callables from
+/// C (https://dartbug.com/61887), so we can't use `xDestroy` callbacks from
+/// SQLite.
+///
+/// Instead, we:
+///
+///  - Manually close callables when the database is closed in Dart.
+///  - Use (regular, non-native) finalizers to asynchronously close callbacks
+///    for databases that haven't been closed manually.
+final class _FunctionFinalizers {
+  final List<NativeCallable> _callables = [];
+
+  void closeAll() {
+    for (final callable in _callables) {
+      callable.close();
+    }
+  }
+
+  static final Finalizer<_FunctionFinalizers> finalizer = Finalizer(
+    (f) => f.closeAll(),
+  );
+}
+
+final class FfiDatabase implements RawSqliteDatabase, Finalizable {
   final Pointer<sqlite3> db;
+  final _FunctionFinalizers _functions = _FunctionFinalizers();
+  final Object _detachToken = Object();
 
   NativeCallable<_UpdateHook>? _installedUpdateHook;
   NativeCallable<_CommitHook>? _installedCommitHook;
   NativeCallable<_RollbackHook>? _installedRollbackHook;
 
-  FfiDatabase(this.db);
+  FfiDatabase(this.db) {
+    databaseFinalizer.attach(this, db.cast(), detach: _detachToken);
+    _FunctionFinalizers.finalizer.attach(
+      this,
+      _functions,
+      detach: _detachToken,
+    );
+  }
 
   @override
   int sqlite3_close_v2() {
-    return libsqlite3.sqlite3_close_v2(db);
+    final rc = libsqlite3.sqlite3_close_v2(db);
+
+    _functions.closeAll();
+    _FunctionFinalizers.finalizer.detach(_detachToken);
+    databaseFinalizer.detach(_detachToken);
+    return rc;
   }
 
   @override
@@ -883,15 +930,15 @@ final class FfiDatabase implements RawSqliteDatabase {
     required RawCollation collation,
   }) {
     final name = allocateBytes(collationName, additionalLength: 1);
-    final compare = collation.toNative();
+    final compare = collation.toNative(_functions);
 
     final result = libsqlite3.sqlite3_create_collation_v2(
       db,
       name.cast(),
       eTextRep,
       nullPtr(),
       compare.nativeFunction,
-      _xDestroy([compare]),
+      nullPtr(),
     );
     name.free();
 
@@ -910,10 +957,10 @@ final class FfiDatabase implements RawSqliteDatabase {
   }) {
     final functionNamePtr = allocateBytes(functionName, additionalLength: 1);
 
-    final step = xStep.toNative();
-    final $final = xFinal.toNative(true);
-    final value = xValue.toNative(false);
-    final inverse = xInverse.toNative();
+    final step = xStep.toNative(_functions);
+    final $final = xFinal.toNative(clean: true, finalizers: _functions);
+    final value = xValue.toNative(clean: false, finalizers: _functions);
+    final inverse = xInverse.toNative(_functions);
 
     final result = libsqlite3.sqlite3_create_window_function(
       db,
@@ -925,7 +972,7 @@ final class FfiDatabase implements RawSqliteDatabase {
       $final.nativeFunction,
       value.nativeFunction,
       inverse.nativeFunction,
-      _xDestroy([step, $final, value, inverse]),
+      nullPtr(),
     );
     functionNamePtr.free();
     return result;
@@ -942,9 +989,9 @@ final class FfiDatabase implements RawSqliteDatabase {
   }) {
     final functionNamePtr = allocateBytes(functionName, additionalLength: 1);
 
-    final func = xFunc?.toNative();
-    final step = xStep?.toNative();
-    final $final = xFinal?.toNative(true);
+    final func = xFunc?.toNative(_functions);
+    final step = xStep?.toNative(_functions);
+    final $final = xFinal?.toNative(clean: true, finalizers: _functions);
 
     final result = libsqlite3.sqlite3_create_function_v2(
       db,
@@ -955,11 +1002,7 @@ final class FfiDatabase implements RawSqliteDatabase {
       func?.nativeFunction ?? nullPtr(),
       step?.nativeFunction ?? nullPtr(),
       $final?.nativeFunction ?? nullPtr(),
-      _xDestroy([
-        if (func != null) func,
-        if (step != null) step,
-        if ($final != null) $final,
-      ]),
+      nullPtr(),
     );
     functionNamePtr.free();
     return result;
@@ -973,7 +1016,7 @@ final class FfiDatabase implements RawSqliteDatabase {
       _installedUpdateHook = null;
       libsqlite3.sqlite3_update_hook(db, nullPtr(), nullPtr());
     } else {
-      final native = _installedUpdateHook = hook.toNative();
+      final native = _installedUpdateHook = hook.toNative(_functions);
       libsqlite3.sqlite3_update_hook(db, native.nativeFunction, nullPtr());
     }
 
@@ -988,7 +1031,7 @@ final class FfiDatabase implements RawSqliteDatabase {
       _installedCommitHook = null;
       libsqlite3.sqlite3_commit_hook(db, nullPtr(), nullPtr());
     } else {
-      final native = _installedCommitHook = hook.toNative();
+      final native = _installedCommitHook = hook.toNative(_functions);
       libsqlite3.sqlite3_commit_hook(db, native.nativeFunction, nullPtr());
     }
 
@@ -1002,7 +1045,7 @@ final class FfiDatabase implements RawSqliteDatabase {
     if (hook == null) {
       libsqlite3.sqlite3_rollback_hook(db, nullPtr(), nullPtr());
     } else {
-      final native = _installedRollbackHook = hook.toNative();
+      final native = _installedRollbackHook = hook.toNative(_functions);
       libsqlite3.sqlite3_rollback_hook(db, native.nativeFunction, nullPtr());
     }
 
@@ -1024,23 +1067,6 @@ final class FfiDatabase implements RawSqliteDatabase {
   RawStatementCompiler newCompiler(List<int> utf8EncodedSql) {
     return FfiStatementCompiler(this, allocateBytes(utf8EncodedSql));
   }
-
-  static Pointer<NativeFunction<Void Function(Pointer<Void>)>> _xDestroy(
-    List<NativeCallable> callables,
-  ) {
-    void destroy(Pointer<Void> _) {
-      for (final callable in callables) {
-        callable.close();
-      }
-    }
-
-    final callable = NativeCallable<Void Function(Pointer<Void>)>.isolateLocal(
-      destroy,
-    )..keepIsolateAlive = false;
-    callables.add(callable);
-
-    return callable.nativeFunction;
-  }
 }
 
 final class FfiStatementCompiler implements RawStatementCompiler {
@@ -1101,12 +1127,15 @@ final class FfiStatementCompiler implements RawStatementCompiler {
   }
 }
 
-final class FfiStatement implements RawSqliteStatement {
+final class FfiStatement implements RawSqliteStatement, Finalizable {
   final Pointer<sqlite3_stmt> stmt;
+  final Object _detachToken = Object();
 
   final List<Pointer> _allocatedArguments = [];
 
-  FfiStatement(this.stmt);
+  FfiStatement(this.stmt) {
+    statementFinalizer.attach(this, stmt.cast(), detach: _detachToken);
+  }
 
   @visibleForTesting
   List<Pointer> get allocatedArguments => _allocatedArguments;
@@ -1251,6 +1280,7 @@ final class FfiStatement implements RawSqliteStatement {
   @override
   void sqlite3_finalize() {
     libsqlite3.sqlite3_finalize(stmt);
+    statementFinalizer.detach(_detachToken);
   }
 
   @override
@@ -1455,72 +1485,93 @@ typedef _UpdateHook =
 typedef _CommitHook = Int Function(Pointer<Void>);
 typedef _RollbackHook = Void Function(Pointer<Void>);
 
+extension on NativeCallable {
+  void closeIn(_FunctionFinalizers finalizers) {
+    finalizers._callables.add(this);
+  }
+}
+
 extension on RawXFunc {
-  NativeCallable<_XFunc> toNative() {
+  NativeCallable<_XFunc> toNative(_FunctionFinalizers finalizers) {
     return NativeCallable.isolateLocal((
-      Pointer<sqlite3_context> ctx,
-      int nArgs,
-      Pointer<Pointer<sqlite3_value>> args,
-    ) {
-      this(FfiContext(ctx), _ValueList(nArgs, args));
-    })..keepIsolateAlive = false;
+        Pointer<sqlite3_context> ctx,
+        int nArgs,
+        Pointer<Pointer<sqlite3_value>> args,
+      ) {
+        this(FfiContext(ctx), _ValueList(nArgs, args));
+      })
+      ..closeIn(finalizers)
+      ..keepIsolateAlive = false;
   }
 }
 
 extension on RawXFinal {
-  NativeCallable<_XFinal> toNative(bool clean) {
+  NativeCallable<_XFinal> toNative({
+    required bool clean,
+    required _FunctionFinalizers finalizers,
+  }) {
     return NativeCallable.isolateLocal((Pointer<sqlite3_context> ctx) {
-      final context = FfiContext(ctx);
-      this(context);
-      if (clean) context.freeContext();
-    })..keepIsolateAlive = false;
+        final context = FfiContext(ctx);
+        this(context);
+        if (clean) context.freeContext();
+      })
+      ..closeIn(finalizers)
+      ..keepIsolateAlive = false;
   }
 }
 
 extension on RawCollation {
-  NativeCallable<_XCompare> toNative() {
+  NativeCallable<_XCompare> toNative(_FunctionFinalizers finalizers) {
     return NativeCallable.isolateLocal((
-      Pointer<Void> _,
-      int lengthA,
-      Pointer<Void> a,
-      int lengthB,
-      Pointer<Void> b,
-    ) {
-      final dartA = a.cast<sqlite3_char>().readNullableString(lengthA);
-      final dartB = b.cast<sqlite3_char>().readNullableString(lengthB);
-
-      return this(dartA, dartB);
-    }, exceptionalReturn: 0)..keepIsolateAlive = false;
+        Pointer<Void> _,
+        int lengthA,
+        Pointer<Void> a,
+        int lengthB,
+        Pointer<Void> b,
+      ) {
+        final dartA = a.cast<sqlite3_char>().readNullableString(lengthA);
+        final dartB = b.cast<sqlite3_char>().readNullableString(lengthB);
+
+        return this(dartA, dartB);
+      }, exceptionalReturn: 0)
+      ..closeIn(finalizers)
+      ..keepIsolateAlive = false;
   }
 }
 
 extension on RawUpdateHook {
-  NativeCallable<_UpdateHook> toNative() {
+  NativeCallable<_UpdateHook> toNative(_FunctionFinalizers finalizers) {
     return NativeCallable.isolateLocal((
-      Pointer<Void> _,
-      int kind,
-      Pointer<sqlite3_char> db,
-      Pointer<sqlite3_char> table,
-      int rowid,
-    ) {
-      final tableName = table.readString();
-      this(kind, tableName, rowid);
-    })..keepIsolateAlive = false;
+        Pointer<Void> _,
+        int kind,
+        Pointer<sqlite3_char> db,
+        Pointer<sqlite3_char> table,
+        int rowid,
+      ) {
+        final tableName = table.readString();
+        this(kind, tableName, rowid);
+      })
+      ..closeIn(finalizers)
+      ..keepIsolateAlive = false;
   }
 }
 
 extension on RawCommitHook {
-  NativeCallable<_CommitHook> toNative() {
+  NativeCallable<_CommitHook> toNative(_FunctionFinalizers finalizers) {
     return NativeCallable.isolateLocal((Pointer<Void> _) {
-      return this();
-    }, exceptionalReturn: 1)..keepIsolateAlive = false;
+        return this();
+      }, exceptionalReturn: 1)
+      ..closeIn(finalizers)
+      ..keepIsolateAlive = false;
   }
 }
 
 extension on RawRollbackHook {
-  NativeCallable<_RollbackHook> toNative() {
+  NativeCallable<_RollbackHook> toNative(_FunctionFinalizers finalizers) {
     return NativeCallable.isolateLocal((Pointer<Void> _) {
-      this();
-    })..keepIsolateAlive = false;
+        this();
+      })
+      ..closeIn(finalizers)
+      ..keepIsolateAlive = false;
   }
 }