diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index c27773e..153674d 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -12,6 +12,7 @@ jobs: if: github.event_name == 'push' || (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.repository) outputs: dart-version: ${{ steps.dart.outputs.dart-version }} + pubspec-hash: ${{ steps.pubspec-hash.outputs.hash }} steps: - uses: actions/checkout@v6 - uses: dart-lang/setup-dart@v1 @@ -20,6 +21,11 @@ jobs: # Make pub cache folder consistent across OSes, so that we can share the cache. run: "echo PUB_CACHE=.dart_tool/pub-cache/ >> $GITHUB_ENV" shell: bash + - name: "Hash pubspec.yaml" + id: pubspec-hash + run: | + echo "hash=${{ hashFiles('pubspec.yaml') }}" >> $GITHUB_OUTPUT + shell: bash # We need to update the cache whenever the pubspec.lock changes, as that # indicates a changed dependency after `pub upgrade`. However, we can't # include the pubspec.lock in the cache key as it's not part of the repository. @@ -30,7 +36,7 @@ jobs: path: | ${{ env.PUB_CACHE }} pubspec.lock - key: dart-deps-${{ steps.dart.outputs.dart-version }}-${{ hashFiles('pubspec.yaml') }} + key: dart-deps-${{ steps.dart.outputs.dart-version }}-${{ steps.pubspec-hash.outputs.hash }} restore-keys: dart-deps-${{ steps.dart.outputs.dart-version }} dart-deps- @@ -59,15 +65,19 @@ jobs: path: | ${{ env.PUB_CACHE }} pubspec.lock - key: dart-deps-${{ steps.dart.outputs.dart-version }}-${{ hashFiles('pubspec.yaml') }} + key: dart-deps-${{ steps.dart.outputs.dart-version }}-${{ steps.pubspec-hash.outputs.hash }} enableCrossOsArchive: true test: needs: [analyze] - runs-on: ubuntu-latest - if: github.event_name == 'push' || (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.repository) + runs-on: ${{ matrix.os }} + if: github.event_name == 'push' || (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.repository) + strategy: + fail-fast: false + matrix: + os: [ubuntu-latest, windows-latest, macos-latest] steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 - uses: dart-lang/setup-dart@v1 with: sdk: ${{ needs.analyze.outputs.dart-version }} @@ -79,7 +89,7 @@ jobs: path: | ${{ env.PUB_CACHE }} pubspec.lock - key: dart-deps-${{ needs.analyze.outputs.dart-version }}-${{ hashFiles('pubspec.yaml') }} + key: dart-deps-${{ needs.analyze.outputs.dart-version }}-${{ needs.analyze.outputs.pubspec-hash }} # Should be created by analyze run fail-on-cache-miss: true enableCrossOsArchive: true @@ -90,14 +100,19 @@ jobs: dart pub global activate pana - name: Setup + run: dart run tool/sqlite3_wasm_download.dart + + - name: Test sqlite_async on Dart VM + working-directory: packages/sqlite_async run: | - dart run tool/sqlite3_wasm_download.dart - dart compile js -O4 --no-minify -Dsqlite3.dartbigints=false -o assets/db_worker.js packages/sqlite_async/lib/src/web/worker/worker.dart + dart test -p vm - - name: Test sqlite_async + - name: Test sqlite_async on Chrome + if: runner.os == 'Linux' working-directory: packages/sqlite_async run: | - dart test -p chrome,vm --compiler dart2js,dart2wasm + dart compile js -O4 --no-minify -Dsqlite3.dartbigints=false -o ../../assets/db_worker.js lib/src/web/worker/worker.dart + dart test -p chrome --compiler dart2js,dart2wasm dart run build_runner test -- -p chrome - name: Test drift_sqlite_async @@ -105,6 +120,7 @@ jobs: run: dart test - name: Pana for sqlite_async + if: runner.os == 'Linux' run: dart pub global run pana --no-warning packages/sqlite_async # We don't run pana on drift_sqlite_async because it can depend on unpublished changes from # sqlite_async, causing it to fail all the time. diff --git a/packages/drift_sqlite_async/test/utils/test_utils.dart b/packages/drift_sqlite_async/test/utils/test_utils.dart index 3cd2dd4..837bb87 100644 --- a/packages/drift_sqlite_async/test/utils/test_utils.dart +++ b/packages/drift_sqlite_async/test/utils/test_utils.dart @@ -1,12 +1,14 @@ import 'dart:io'; import 'package:sqlite_async/sqlite_async.dart'; +import 'package:test_api/scaffolding.dart'; import 'package:test_api/src/backend/invoker.dart'; Future setupDatabase({String? path}) async { final db = SqliteDatabase.withFactory(SqliteOpenFactory(path: path ?? dbPath())); await db.initialize(); + addTearDown(db.close); return db; } diff --git a/packages/sqlite_async/CHANGELOG.md b/packages/sqlite_async/CHANGELOG.md index 9d294d4..3675c8b 100644 --- a/packages/sqlite_async/CHANGELOG.md +++ b/packages/sqlite_async/CHANGELOG.md @@ -1,3 +1,7 @@ +## 0.14.5 + +- Make `close()` wait for the database to actually be closed. + ## 0.14.4 - Native: Add the `NativeSqliteOpenFactory.beforeOpen` method, which can be overridden to configure diff --git a/packages/sqlite_async/lib/src/native/database/native_sqlite_database.dart b/packages/sqlite_async/lib/src/native/database/native_sqlite_database.dart index ea6f14d..e718f48 100644 --- a/packages/sqlite_async/lib/src/native/database/native_sqlite_database.dart +++ b/packages/sqlite_async/lib/src/native/database/native_sqlite_database.dart @@ -40,7 +40,7 @@ final class NativeSqliteDatabaseImpl extends SqliteDatabaseImpl { return pool; }); - bool _isClosed = false; + Future? _closing; final _lockGuard = Object(); @override @@ -66,7 +66,7 @@ final class NativeSqliteDatabaseImpl extends SqliteDatabaseImpl { @override bool get closed { - return _isClosed; + return _closing != null; } /// Returns true if the _write_ connection is in auto-commit mode @@ -84,14 +84,20 @@ final class NativeSqliteDatabaseImpl extends SqliteDatabaseImpl { } @override - Future close() async { - _isClosed = true; - final pool = await _pool; - pool.close(); + Future close() { + _checkNotLocked('close'); - while (_workers.isNotEmpty) { - _workers.removeFirst().close(); - } + return _closing ??= Future.sync(() async { + final pool = await _pool; + + // Acquire all connections to ensure this doesn't race with any leased + // connection. + final allConnections = await pool.exclusiveAccess(); + pool.close(); // Prevent subsequent pool requests. + + await _workers.map((e) => e.close()).wait; + allConnections.close(); + }); } @override @@ -224,11 +230,7 @@ final class NativeSqliteDatabaseImpl extends SqliteDatabaseImpl { } void _returnIsolateWorker(IsolateWorker worker) { - if (_isClosed) { - worker.close(); - } else { - _workers.addLast(worker); - } + _workers.addLast(worker); } void _checkNotLocked(String? debugContext) { diff --git a/packages/sqlite_async/lib/src/native/database/worker.dart b/packages/sqlite_async/lib/src/native/database/worker.dart index c488e74..7f52937 100644 --- a/packages/sqlite_async/lib/src/native/database/worker.dart +++ b/packages/sqlite_async/lib/src/native/database/worker.dart @@ -11,14 +11,30 @@ import 'package:async/async.dart'; final class IsolateWorker { final Isolate _isolate; - final ReceivePort _receiveResponses = ReceivePort('isolate worker'); + final ReceivePort _receiveResponses; final SendPort _sendCommands; final Map> _outstandingWorkItems = {}; int _nextWorkItem = 0; + var _closeRequested = false; + late final Future _closeAcknowledged; + + IsolateWorker._(this._isolate, this._sendCommands, this._receiveResponses) { + _closeAcknowledged = _receiveResponses.listen((Object? message) { + if (message == null) { + // Null message is the exit listener registered on the isolate. + + for (final pending in _outstandingWorkItems.values) { + // This really shouldn't happen, but it's better than not having a future + // that doesn't complete. + pending.completeError(StateError('Worker closed')); + } + _outstandingWorkItems.clear(); + + _receiveResponses.close(); + return; + } - IsolateWorker._(this._isolate, this._sendCommands) { - _receiveResponses.listen((Object? message) { final WorkResult(:id, :result) = message as WorkResult; if (_outstandingWorkItems.remove(id) case final completer?) { switch (result) { @@ -28,10 +44,12 @@ final class IsolateWorker { completer.completeError(error, stackTrace); } } - }); + }).asFuture(); } Future run(FutureOr Function() task) async { + if (_closeRequested) throw StateError('Isolate worker closed'); + final id = _nextWorkItem++; final completer = _outstandingWorkItems[id] = Completer(); @@ -39,23 +57,25 @@ final class IsolateWorker { return (await completer.future) as T; } - void close() { + Future close() { + _closeRequested = true; _isolate.kill(); - for (final pending in _outstandingWorkItems.values) { - // This really shouldn't happen, but it's better than not having a future - // that doesn't complete. - pending.completeError(StateError('Worker closed')); - } - _outstandingWorkItems.clear(); - _receiveResponses.close(); + + return _closeAcknowledged; } static Future spawn() async { final receiveSendPort = ReceivePort(); - final isolate = await Isolate.spawn(_entrypoint, receiveSendPort.sendPort); + final receiveResponses = ReceivePort('isolate worker'); + final isolate = await Isolate.spawn( + _entrypoint, + receiveSendPort.sendPort, + onExit: receiveResponses.sendPort, + ); + final port = (await receiveSendPort.first) as SendPort; - return IsolateWorker._(isolate, port); + return IsolateWorker._(isolate, port, receiveResponses); } static void _entrypoint(SendPort sendPort) async { diff --git a/packages/sqlite_async/pubspec.yaml b/packages/sqlite_async/pubspec.yaml index eaa54a1..0d0105a 100644 --- a/packages/sqlite_async/pubspec.yaml +++ b/packages/sqlite_async/pubspec.yaml @@ -1,6 +1,6 @@ name: sqlite_async description: High-performance asynchronous interface for SQLite on Dart and Flutter. -version: 0.14.4 +version: 0.14.5 resolution: workspace repository: https://github.com/powersync-ja/sqlite_async.dart environment: diff --git a/packages/sqlite_async/test/basic_test.dart b/packages/sqlite_async/test/basic_test.dart index 52b4cdd..b15c4bb 100644 --- a/packages/sqlite_async/test/basic_test.dart +++ b/packages/sqlite_async/test/basic_test.dart @@ -303,7 +303,7 @@ void main() { final hasLock = Completer(); final db = await testUtils.setupDatabase(path: path); - db.withAllConnections((writer, readers) async { + final allConnections = db.withAllConnections((writer, readers) async { hasLock.complete(); await releaseLock.future; }); @@ -318,6 +318,9 @@ void main() { db.abortableWriteLock((_) async {}, abortTrigger: Future.value(null)), throwsAbortException, ); + + releaseLock.complete(); + await allConnections; }); test('execute single statement with RETURNING populates ResultSet', @@ -445,6 +448,7 @@ void main() { await testUtils.testFactory( path: path, options: SqliteOptions(maxReaders: maxReaders)), ); + addTearDown(db.close); await db.initialize(); await createTables(db); @@ -512,6 +516,17 @@ void main() { expect(row.values.map((e) => e.runtimeType), [int, double]); }); }, skip: identical(0, 0.0) ? 'Requires 64-bit ints' : false); + + test('cannot use closed databases', () async { + final db = await testUtils.setupDatabase(path: path); + + await db.initialize(); + await db.close(); + expect(db.closed, isTrue); + + await expectLater(db.execute('SELECT 1'), throwsA(anything)); + await expectLater(db.get('SELECT 1'), throwsA(anything)); + }); }); } diff --git a/packages/sqlite_async/test/native/basic_test.dart b/packages/sqlite_async/test/native/basic_test.dart index f6b6315..ffef81d 100644 --- a/packages/sqlite_async/test/native/basic_test.dart +++ b/packages/sqlite_async/test/native/basic_test.dart @@ -58,6 +58,7 @@ void main() { await testUtils.testFactory( path: path, options: SqliteOptions(maxReaders: 3)), ); + addTearDown(db.close); await db.initialize(); await createTables(db); @@ -90,6 +91,7 @@ void main() { await testUtils.testFactory( path: path, options: SqliteOptions(maxReaders: 3)), ); + addTearDown(db.close); await db.initialize(); await createTables(db); @@ -366,6 +368,7 @@ void main() { test('invokes beforeOpen callback on factories', () async { final factoy = _BeforeSetupHook(path: path); final db = SqliteDatabase.withFactory(factoy); + addTearDown(db.close); expect(factoy.didCallBeforeOpen, isFalse); await db.initialize(); diff --git a/packages/sqlite_async/test/native/watch_test.dart b/packages/sqlite_async/test/native/watch_test.dart index 43df54c..31a8953 100644 --- a/packages/sqlite_async/test/native/watch_test.dart +++ b/packages/sqlite_async/test/native/watch_test.dart @@ -41,6 +41,7 @@ void main() { (await testUtils.testFactory(path: path)) as NativeSqliteOpenFactory; final db = factory.openNativeConnection( SqliteOpenOptions(primaryConnection: true, readOnly: false)); + addTearDown(db.close); db.execute('CREATE TABLE a (bar INTEGER);'); db.execute('CREATE TABLE b (bar INTEGER);'); @@ -148,8 +149,8 @@ void main() { final reads = StreamQueue(db.updates); final first = reads.next; - db.writeLock((ctx) async { - await ctx.execute('INSERT INTO customer(name) VALUES (?)', ['test']); + await db.writeLock((ctx) async { + await ctx.execute('INSERT INTO customers(name) VALUES (?)', ['test']); // Because we're not in a transaction, this should emit an update. We // shouldn't just collect updates at the end of writeLock to avoid // long-running writers never emitting updates. diff --git a/packages/sqlite_async/test/utils/abstract_test_utils.dart b/packages/sqlite_async/test/utils/abstract_test_utils.dart index de6dc05..ff6c2e1 100644 --- a/packages/sqlite_async/test/utils/abstract_test_utils.dart +++ b/packages/sqlite_async/test/utils/abstract_test_utils.dart @@ -1,5 +1,6 @@ import 'package:sqlite3/common.dart'; import 'package:sqlite_async/sqlite_async.dart'; +import 'package:test/scaffolding.dart'; abstract class AbstractTestUtils { String dbPath(); @@ -17,7 +18,10 @@ abstract class AbstractTestUtils { SqliteOptions options = defaultTestOptions, }) async { final factory = await testFactory(path: path, options: options); - return SqliteDatabase.withFactory(factory); + final db = SqliteDatabase.withFactory(factory); + + addTearDown(db.close); + return db; } /// Deletes any DB data diff --git a/packages/sqlite_async/test/watch_test.dart b/packages/sqlite_async/test/watch_test.dart index 4743b80..ca199e8 100644 --- a/packages/sqlite_async/test/watch_test.dart +++ b/packages/sqlite_async/test/watch_test.dart @@ -75,7 +75,7 @@ void main() { const numberOfQueries = 10; - inserts(); + final insertsDone = inserts(); try { List times = []; final results = await stream.take(numberOfQueries).map((e) { @@ -110,6 +110,8 @@ void main() { } finally { done = true; } + + await insertsDone; }); test('onChange', () async { @@ -129,7 +131,7 @@ void main() { } } - inserts(); + final insertsDone = inserts(); final stream = db.onChange({'assets', 'customers'}, throttle: throttleDuration).asyncMap((event) async { @@ -147,6 +149,7 @@ void main() { UpdateNotification.single('assets'), UpdateNotification.single('assets') ])); + await insertsDone; }); test('single onChange', () async { @@ -202,7 +205,7 @@ void main() { const numberOfQueries = 10; - inserts(); + final insertsDone = inserts(); try { List times = []; final results = await stream.take(numberOfQueries).map((e) { @@ -235,6 +238,7 @@ void main() { } finally { done = true; } + await insertsDone; }); test('watch with transaction', () async {