Fix statements not getting disposed without cache

drift/CHANGELOG.md

@@ -1,3 +1,8 @@
+## 2.8.2
+
+- Fix prepared statements leaking when the statement cache is disabled.
+- Disable prepared statement caching by default.
+
 ## 2.8.1
 
 - Performance improvement: Cache and re-use prepared statements - thanks to [@davidmartos96](https://github.com/davidmartos96/)

drift/lib/native.dart

@@ -34,6 +34,9 @@ typedef DatabaseSetup = void Function(Database database);
 /// A drift database implementation based on `dart:ffi`, running directly in a
 /// Dart VM or an AOT compiled Dart/Flutter application.
 class NativeDatabase extends DelegatedDatabase {
+  // when changing this, also update the documentation in `drift_vm_database_factory`.
+  static const _cacheStatementsByDefault = false;
+
   NativeDatabase._(DatabaseDelegate delegate, bool logStatements)
       : super(delegate, isSequential: false, logStatements: logStatements);
 
@@ -43,6 +46,12 @@ class NativeDatabase extends DelegatedDatabase {
   /// {@template drift_vm_database_factory}
   /// If [logStatements] is true (defaults to `false`), generated sql statements
   /// will be printed before executing. This can be useful for debugging.
+  ///
+  /// The [cachePreparedStatements] flag (defaults to `false`) controls whether
+  /// drift will cache prepared statement objects, which improves performance as
+  /// sqlite3 doesn't have to parse statements that are frequently used multiple
+  /// times. This will be the default in the next minor drift version.
+  ///
   /// The optional [setup] function can be used to perform a setup just after
   /// the database is opened, before drift is fully ready. This can be used to
   /// add custom user-defined sql functions or to provide encryption keys in
@@ -52,7 +61,7 @@ class NativeDatabase extends DelegatedDatabase {
     File file, {
     bool logStatements = false,
     DatabaseSetup? setup,
-    bool cachePreparedStatements = true,
+    bool cachePreparedStatements = _cacheStatementsByDefault,
   }) {
     return NativeDatabase._(
         _NativeDelegate(
@@ -80,10 +89,18 @@ class NativeDatabase extends DelegatedDatabase {
   /// __Important limitations__: If the [setup] parameter is given, it must be
   /// a static or top-level function. The reason is that it is executed on
   /// another isolate.
-  static QueryExecutor createInBackground(File file,
-      {bool logStatements = false, DatabaseSetup? setup}) {
-    return createBackgroundConnection(file,
-        logStatements: logStatements, setup: setup);
+  static QueryExecutor createInBackground(
+    File file, {
+    bool logStatements = false,
+    bool cachePreparedStatements = _cacheStatementsByDefault,
+    DatabaseSetup? setup,
+  }) {
+    return createBackgroundConnection(
+      file,
+      logStatements: logStatements,
+      setup: setup,
+      cachePreparedStatements: cachePreparedStatements,
+    );
   }
 
   /// Like [createInBackground], except that it returns the whole
@@ -91,14 +108,23 @@ class NativeDatabase extends DelegatedDatabase {
   ///
   /// This creates a database writing data to the given [file]. The database
   /// runs in a background isolate and is stopped when closed.
-  static DatabaseConnection createBackgroundConnection(File file,
-      {bool logStatements = false, DatabaseSetup? setup}) {
+  static DatabaseConnection createBackgroundConnection(
+    File file, {
+    bool logStatements = false,
+    DatabaseSetup? setup,
+    bool cachePreparedStatements = _cacheStatementsByDefault,
+  }) {
     return DatabaseConnection.delayed(Future.sync(() async {
       final receiveIsolate = ReceivePort();
       await Isolate.spawn(
         _NativeIsolateStartup.start,
         _NativeIsolateStartup(
-            file.absolute.path, logStatements, setup, receiveIsolate.sendPort),
+          file.absolute.path,
+          logStatements,
+          cachePreparedStatements,
+          setup,
+          receiveIsolate.sendPort,
+        ),
         debugName: 'Drift isolate worker for ${file.path}',
       );
 
@@ -115,7 +141,7 @@ class NativeDatabase extends DelegatedDatabase {
   factory NativeDatabase.memory({
     bool logStatements = false,
     DatabaseSetup? setup,
-    bool cachePreparedStatements = true,
+    bool cachePreparedStatements = _cacheStatementsByDefault,
   }) {
     return NativeDatabase._(
       _NativeDelegate(null, setup, cachePreparedStatements),
@@ -140,7 +166,7 @@ class NativeDatabase extends DelegatedDatabase {
     bool logStatements = false,
     DatabaseSetup? setup,
     bool closeUnderlyingOnClose = true,
-    bool cachePreparedStatements = true,
+    bool cachePreparedStatements = _cacheStatementsByDefault,
   }) {
     return NativeDatabase._(
         _NativeDelegate.opened(
@@ -295,17 +321,24 @@ class _NativeDelegate extends Sqlite3Delegate<Database> {
 class _NativeIsolateStartup {
   final String path;
   final bool enableLogs;
+  final bool cachePreparedStatements;
   final DatabaseSetup? setup;
   final SendPort sendServer;
 
   _NativeIsolateStartup(
-      this.path, this.enableLogs, this.setup, this.sendServer);
+    this.path,
+    this.enableLogs,
+    this.cachePreparedStatements,
+    this.setup,
+    this.sendServer,
+  );
 
   static void start(_NativeIsolateStartup startup) {
     final isolate = DriftIsolate.inCurrent(() {
       return DatabaseConnection(NativeDatabase(
         File(startup.path),
         logStatements: startup.enableLogs,
+        cachePreparedStatements: startup.cachePreparedStatements,
         setup: startup.setup,
       ));
     });

drift/lib/src/sqlite3/database.dart

@@ -138,15 +138,24 @@ abstract class Sqlite3Delegate<DB extends CommonDatabase>
       database.execute(statement);
     } else {
       final stmt = _getPreparedStatement(statement);
-      stmt.execute(args);
+      try {
+        stmt.execute(args);
+      } finally {
+        _returnStatement(stmt);
+      }
     }
   }
 
   @override
   Future<QueryResult> runSelect(String statement, List<Object?> args) async {
     final stmt = _getPreparedStatement(statement);
-    final result = stmt.select(args);
-    return QueryResult.fromRows(result.toList());
+
+    try {
+      final result = stmt.select(args);
+      return QueryResult.fromRows(result.toList());
+    } finally {
+      _returnStatement(stmt);
+    }
   }
 
   CommonPreparedStatement _getPreparedStatement(String sql) {
@@ -165,6 +174,14 @@ abstract class Sqlite3Delegate<DB extends CommonDatabase>
       return stmt;
     }
   }
+
+  void _returnStatement(CommonPreparedStatement stmt) {
+    // When using a statement cache, prepared statements are disposed as they
+    // get evicted from the cache, so we don't need to do anything.
+    if (!cachePreparedStatements) {
+      stmt.dispose();
+    }
+  }
 }
 
 class _SqliteVersionDelegate extends DynamicVersionDelegate {
@@ -186,6 +203,12 @@ class _SqliteVersionDelegate extends DynamicVersionDelegate {
 /// multiple time when they're used frequently.
 @internal
 class PreparedStatementsCache {
+  /// The default amount of prepared statements to keep cached.
+  ///
+  /// This value is used in tests to verify that evicted statements get disposed.
+  @visibleForTesting
+  static const defaultSize = 64;
+
   /// The maximum amount of cached statements.
   final int maxSize;
 
@@ -197,7 +220,7 @@ class PreparedStatementsCache {
       LinkedHashMap();
 
   /// Create a cache of prepared statements with a capacity of [maxSize].
-  PreparedStatementsCache({this.maxSize = 64});
+  PreparedStatementsCache({this.maxSize = defaultSize});
 
   /// Attempts to look up the cached [sql] statement, if it exists.
   ///

drift/test/platforms/ffi/native_database_test.dart

@@ -1,8 +1,11 @@
 @TestOn('vm')
+import 'dart:ffi';
 import 'dart:io';
 
 import 'package:drift/drift.dart';
 import 'package:drift/native.dart';
+import 'package:drift/src/sqlite3/database.dart';
+import 'package:sqlite3/open.dart';
 import 'package:sqlite3/sqlite3.dart';
 import 'package:test/test.dart';
 import 'package:test_descriptor/test_descriptor.dart' as d;
@@ -120,6 +123,47 @@ void main() {
     // Should also prevent subsequent open attempts
     await expectLater(db.ensureOpen(_FakeExecutorUser()), throwsA(exception));
   });
+
+  test('disposes statements directly if cache is disabled', () async {
+    final underlying = sqlite3.openInMemory();
+    final db =
+        NativeDatabase.opened(underlying, cachePreparedStatements: false);
+    addTearDown(db.close);
+
+    await db.ensureOpen(_FakeExecutorUser());
+    await db.runCustom('CREATE TABLE foo (bar INTEGER);');
+    await db.runCustom('UPDATE foo SET bar = 1');
+    expect(underlying.activeStatementCount, isZero);
+  });
+
+  test('disposes statements eventually if cache is enabled', () async {
+    final underlying = sqlite3.openInMemory();
+    addTearDown(underlying.dispose);
+    final db = NativeDatabase.opened(
+      underlying,
+      cachePreparedStatements: true,
+      closeUnderlyingOnClose: false,
+    );
+
+    await db.ensureOpen(_FakeExecutorUser());
+    expect(underlying.activeStatementCount, isZero);
+
+    for (var i = 1; i <= PreparedStatementsCache.defaultSize; i++) {
+      await db.runSelect('SELECT $i', []);
+      expect(underlying.activeStatementCount, i);
+    }
+
+    for (var i = 0; i < PreparedStatementsCache.defaultSize; i++) {
+      // This will evict old statements, so the amount of active statements
+      // should not change.
+      await db.runSelect('SELECT $i AS "new"', []);
+      expect(
+          underlying.activeStatementCount, PreparedStatementsCache.defaultSize);
+    }
+
+    await db.close();
+    expect(underlying.activeStatementCount, isZero);
+  });
 }
 
 class _FakeExecutorUser extends QueryExecutorUser {
@@ -131,3 +175,26 @@ class _FakeExecutorUser extends QueryExecutorUser {
   @override
   int get schemaVersion => 1;
 }
+
+extension on Database {
+  /// Counts the amount of statements currently prepared in this database by
+  /// invoking `sqlite3_next_stmt` repeatedly.
+  int get activeStatementCount {
+    final library = open.openSqlite();
+    final nextStmt = library.lookupFunction<Pointer Function(Pointer, Pointer),
+        Pointer Function(Pointer, Pointer)>(
+      'sqlite3_next_stmt',
+    );
+
+    Pointer currentStatement;
+    var count = 0;
+
+    for (currentStatement = nextStmt(handle.cast(), nullptr);
+        currentStatement.address != 0;
+        currentStatement = nextStmt(handle.cast(), currentStatement)) {
+      count++;
+    }
+
+    return count;
+  }
+}