From f295af1398e63875d5da0b02b91d5ee315f6e7ab Mon Sep 17 00:00:00 2001 From: Simon Binder Date: Wed, 30 Sep 2020 15:39:02 +0200 Subject: [PATCH] Add closeExistingInstances api for #835 --- moor/CHANGELOG.md | 3 +- moor/lib/src/ffi/database_tracker.dart | 73 ++++++++++++++++++++++++++ moor/lib/src/ffi/vm_database.dart | 47 +++++++++++++++++ moor/pubspec.yaml | 2 +- moor/test/ffi/close_existing_test.dart | 40 ++++++++++++++ 5 files changed, 163 insertions(+), 2 deletions(-) create mode 100644 moor/lib/src/ffi/database_tracker.dart create mode 100644 moor/test/ffi/close_existing_test.dart diff --git a/moor/CHANGELOG.md b/moor/CHANGELOG.md index e9c89218..10afea45 100644 --- a/moor/CHANGELOG.md +++ b/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 diff --git a/moor/lib/src/ffi/database_tracker.dart b/moor/lib/src/ffi/database_tracker.dart new file mode 100644 index 00000000..97a5df7a --- /dev/null +++ b/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;'); + } + } +} diff --git a/moor/lib/src/ffi/vm_database.dart b/moor/lib/src/ffi/vm_database.dart index d2b70bb2..3463917d 100644 --- a/moor/lib/src/ffi/vm_database.dart +++ b/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 close() async { _db.dispose(); + tracker.markClosed(_db); } } diff --git a/moor/pubspec.yaml b/moor/pubspec.yaml index bbef5dc7..142c1b55 100644 --- a/moor/pubspec.yaml +++ b/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 diff --git a/moor/test/ffi/close_existing_test.dart b/moor/test/ffi/close_existing_test.dart new file mode 100644 index 00000000..b43d8262 --- /dev/null +++ b/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 beforeOpen(QueryExecutor executor, OpeningDetails details) { + return Future.value(); + } + + @override + int get schemaVersion => 1; +}