Add closeExistingInstances api for #835

moor/CHANGELOG.md

@@ -11,7 +11,8 @@
   This generates an optional named parameter in Dart.
 - New `generate_values_in_copy_with` [build option](https://moor.simonbinder.eu/docs/advanced-features/builder_options/).
   It wraps nullable columns in a `Value` in `copyWith` methods so that they can be set to `null`.
-- Add `groupConcat`, `cast` and `coalesce` functions to the Dart query builder.
+- Added `groupConcat`, `cast` and `coalesce` functions to the Dart query builder.
+- Added `VmDatabase.closeExistingInstances()` to close zombie database connections after hot restarts.
 
 ## 3.3.1
 

moor/lib/src/ffi/database_tracker.dart

@@ -0,0 +1,73 @@
+import 'dart:ffi';
+
+import 'package:sqlite3/sqlite3.dart';
+
+/// This entire file is an elaborate hack to workaround https://github.com/simolus3/moor/issues/835.
+///
+/// Users were running into database deadlocks after (stateless) hot restarts
+/// in Flutter when they use transactions. The problem is that we don't have a
+/// chance to call `sqlite3_close` before a Dart VM restart, the Dart object is
+/// just gone without a trace. This means that we're leaking sqlite3 database
+/// connections on restarts.
+/// Even worse, those connections might have a lock on the database, for
+/// instance if they just started a transaction.
+///
+/// Our solution is to store open sqlite3 database connections in an in-memory
+/// sqlite database which can survive restarts! For now, we keep track of the
+/// pointer of an sqlite3 database handle in that database.
+/// At an early stage of their `main()` method, users can now use
+/// `VmDatabase.closeExistingInstances()` to release those resources.
+final DatabaseTracker tracker = DatabaseTracker();
+
+/// Internal class that we don't export to moor users. See [tracker] for why
+/// this is necessary.
+class DatabaseTracker {
+  final Database _db;
+
+  /// Creates a new tracker with necessary tables.
+  DatabaseTracker()
+      : _db = sqlite3.open(
+          'file:moor_connection_store?mode=memory&cache=shared',
+          uri: true,
+        ) {
+    _db.execute('''
+CREATE TABLE IF NOT EXISTS open_connections(
+  database_pointer INTEGER NOT NULL PRIMARY KEY,
+  path TEXT NULL
+);
+    ''');
+  }
+
+  /// Tracks the [openedDb]. The [path] argument can be used to track the path
+  /// of that database, if it's bound to a file.
+  void markOpened(String path, Database openedDb) {
+    final stmt = _db.prepare('INSERT INTO open_connections VALUES (?, ?)');
+    stmt.execute([openedDb.handle.address, path]);
+    stmt.dispose();
+  }
+
+  /// Marks the database [db] as closed.
+  void markClosed(Database db) {
+    final ptr = db.handle.address;
+    _db.execute('DELETE FROM open_connections WHERE database_pointer = $ptr');
+  }
+
+  /// Closes tracked database connections.
+  void closeExisting() {
+    _db.execute('BEGIN;');
+
+    try {
+      final results =
+          _db.select('SELECT database_pointer FROM open_connections');
+
+      for (final row in results) {
+        final ptr = Pointer.fromAddress(row.columnAt(0) as int);
+        sqlite3.fromPointer(ptr).dispose();
+      }
+
+      _db.execute('DELETE FROM open_connections;');
+    } finally {
+      _db.execute('COMMIT;');
+    }
+  }
+}

moor/lib/src/ffi/vm_database.dart

@@ -1,8 +1,11 @@
 import 'dart:io';
 
+import 'package:meta/meta.dart';
 import 'package:moor/backends.dart';
+import 'package:moor/moor.dart';
 import 'package:sqlite3/sqlite3.dart';
 
+import 'database_tracker.dart';
 import 'moor_ffi_functions.dart';
 
 /// Signature of a function that can perform setup work on a [database] before
@@ -42,6 +45,48 @@ class VmDatabase extends DelegatedDatabase {
   factory VmDatabase.memory({bool logStatements = false, DatabaseSetup setup}) {
     return VmDatabase._(_VmDelegate(null, setup), logStatements);
   }
+
+  /// Disposes resources allocated by all `VmDatabase` instances of this
+  /// process.
+  ///
+  /// This method will call `sqlite3_close_v2` for every `VmDatabase` that this
+  /// process has opened without closing later.
+  /// Ideally, all databases should be closed properly in Dart. In that case,
+  /// it's not necessary to call [closeExistingInstances]. However, features
+  /// like hot (stateless) restart can make it impossible to reliably close
+  /// every database. In that case, we leak native sqlite3 database connections
+  /// that aren't referenced by any Dart object. Moor can track those
+  /// connections across Dart VM restarts by storing them in an in-memory sqlite
+  /// database.
+  /// Calling this method can cleanup resources and database locks after a
+  /// restart.
+  ///
+  /// Note that calling [closeExistingInstances] when you're still actively
+  /// using a [VmDatabase] can lead to crashes, since the database would then
+  /// attempt to use an invalid connection.
+  /// This, this method should only be called when you're certain that there
+  /// aren't any active [VmDatabase]s, not even on another isolate.
+  ///
+  /// A suitable place to call [closeExistingInstances] is at an early stage
+  /// of your `main` method, before you're using moor.
+  ///
+  /// ```dart
+  /// void main() {
+  ///   // Guard against zombie database connections caused by hot restarts
+  ///   assert(() {
+  ///     VmDatabase.closeExistingInstances();
+  ///     return true;
+  ///   }());
+  ///
+  ///   runApp(MyApp());
+  /// }
+  /// ```
+  ///
+  /// For more information, see [issue 835](https://github.com/simolus3/moor/issues/835).
+  @experimental
+  static void closeExistingInstances() {
+    tracker.closeExisting();
+  }
 }
 
 class _VmDelegate extends DatabaseDelegate {
@@ -72,6 +117,7 @@ class _VmDelegate extends DatabaseDelegate {
       }
 
       _db = sqlite3.open(file.path);
+      tracker.markOpened(file.path, _db);
     } else {
       _db = sqlite3.openInMemory();
     }
@@ -141,6 +187,7 @@ class _VmDelegate extends DatabaseDelegate {
   @override
   Future<void> close() async {
     _db.dispose();
+    tracker.markClosed(_db);
   }
 }
 

moor/pubspec.yaml

@@ -14,7 +14,7 @@ dependencies:
   collection: ^1.0.0
   synchronized: ^2.1.0
   pedantic: ^1.0.0
-  sqlite3: ^0.1.3
+  sqlite3: ^0.1.6
 
 dev_dependencies:
   moor_generator: ^3.2.0

moor/test/ffi/close_existing_test.dart

@@ -0,0 +1,40 @@
+@TestOn('vm')
+import 'dart:io';
+
+import 'package:moor/ffi.dart';
+import 'package:moor/moor.dart';
+import 'package:test/test.dart';
+import 'package:path/path.dart' as p;
+
+void main() {
+  test('can close lost instances', () async {
+    final file = File(p.join(Directory.systemTemp.path, 'moor_close.db'));
+    if (file.existsSync()) file.deleteSync();
+
+    // Create the first database holding the lock
+    final db1 = VmDatabase(file);
+    await db1.ensureOpen(_NullUser());
+    await db1.runCustom('BEGIN EXCLUSIVE');
+
+    // Close instances indirectly (we don't close db1)
+    VmDatabase.closeExistingInstances();
+
+    // Now open a second instance, it should be able to start a transactions
+    final db2 = VmDatabase(file);
+    await db2.ensureOpen(_NullUser());
+    await db2.runCustom('BEGIN EXCLUSIVE');
+    await db2.runCustom('COMMIT');
+
+    await db2.close();
+  });
+}
+
+class _NullUser extends QueryExecutorUser {
+  @override
+  Future<void> beforeOpen(QueryExecutor executor, OpeningDetails details) {
+    return Future.value();
+  }
+
+  @override
+  int get schemaVersion => 1;
+}