From 0877e761e4dc6b03051ae4d8e71ec71055699760 Mon Sep 17 00:00:00 2001 From: Simon Binder Date: Sat, 3 Feb 2024 17:07:20 +0100 Subject: [PATCH] Create stack trace across remote isolates --- drift/CHANGELOG.md | 2 + drift/lib/src/remote/communication.dart | 20 ++++++++-- drift/lib/src/remote/server_impl.dart | 2 +- drift/pubspec.yaml | 2 +- drift/test/isolate_test.dart | 53 ++++++++++++++++++++++++- 5 files changed, 72 insertions(+), 7 deletions(-) diff --git a/drift/CHANGELOG.md b/drift/CHANGELOG.md index 91122d10..96fd08a2 100644 --- a/drift/CHANGELOG.md +++ b/drift/CHANGELOG.md @@ -9,6 +9,8 @@ `database.update` outside of a transaction and then calling `UpdateStatement.write` inside of a transaction will now perform the update inside of the transaction, instead of causing a deadlock. +- Improve stack traces for errors happening on drift isolates (which includes + usages of `NativeDatabase.createInBackground`). ## 2.15.0 diff --git a/drift/lib/src/remote/communication.dart b/drift/lib/src/remote/communication.dart index 98ab7c48..8994a3e2 100644 --- a/drift/lib/src/remote/communication.dart +++ b/drift/lib/src/remote/communication.dart @@ -2,6 +2,7 @@ import 'dart:async'; import 'package:drift/src/runtime/api/runtime_api.dart'; import 'package:meta/meta.dart'; +import 'package:stack_trace/stack_trace.dart'; import 'package:stream_channel/stream_channel.dart'; import '../runtime/cancellation_zone.dart'; @@ -90,8 +91,7 @@ class DriftCommunication { final request = _pendingRequests.remove(requestId); request?.completeWithError( - DriftRemoteException._(msg.error, msg.stackTrace)); - _pendingRequests.remove(msg.requestId); + DriftRemoteException._(msg.error, msg.stackTrace), msg.stackTrace); } else if (msg is Request) { _incomingRequests.add(msg); } else if (msg is CancelledResponse) { @@ -184,8 +184,15 @@ class _PendingRequest { _PendingRequest(this.completer, this.requestTrace); - void completeWithError(Object error) { - completer.completeError(error, requestTrace); + void completeWithError(Object error, [StackTrace? trace]) { + completer.completeError( + error, + trace == null + ? requestTrace + : Chain([ + if (trace is Chain) ...trace.traces else Trace.from(trace), + Trace.from(requestTrace) + ])); } } @@ -194,6 +201,11 @@ class _PendingRequest { class ConnectionClosedException implements Exception { /// Constant constructor. const ConnectionClosedException(); + + @override + String toString() { + return 'Channel was closed before receiving a response'; + } } /// An exception reported on the other end of a drift remote protocol. diff --git a/drift/lib/src/remote/server_impl.dart b/drift/lib/src/remote/server_impl.dart index 10af3fd6..3b93850c 100644 --- a/drift/lib/src/remote/server_impl.dart +++ b/drift/lib/src/remote/server_impl.dart @@ -9,7 +9,7 @@ import '../runtime/cancellation_zone.dart'; import 'communication.dart'; import 'protocol.dart'; -/// The implementation of a drift server, manging remote channels to send +/// The implementation of a drift server, managing remote channels to send /// database requests. @internal class ServerImplementation implements DriftServer { diff --git a/drift/pubspec.yaml b/drift/pubspec.yaml index 3128992f..8e1976e3 100644 --- a/drift/pubspec.yaml +++ b/drift/pubspec.yaml @@ -17,6 +17,7 @@ dependencies: stream_channel: ^2.1.0 sqlite3: ^2.2.0 path: ^1.8.0 + stack_trace: ^1.11.1 dev_dependencies: archive: ^3.3.1 @@ -35,6 +36,5 @@ dev_dependencies: mockito: ^5.4.3 rxdart: ^0.27.0 shelf: ^1.3.0 - stack_trace: ^1.10.0 test_descriptor: ^2.0.1 vm_service: ^13.0.0 diff --git a/drift/test/isolate_test.dart b/drift/test/isolate_test.dart index 4f87033c..c27c8b2e 100644 --- a/drift/test/isolate_test.dart +++ b/drift/test/isolate_test.dart @@ -9,6 +9,7 @@ import 'package:drift/native.dart'; import 'package:drift/src/isolate.dart'; import 'package:drift/src/remote/communication.dart'; import 'package:mockito/mockito.dart'; +import 'package:stack_trace/stack_trace.dart'; import 'package:test/test.dart'; import 'generated/todos.dart'; @@ -110,7 +111,7 @@ void main() { // The stack trace of remote exceptions should point towards the actual // source making the faulty call. - expect(s.toString(), contains('main.')); + expect(s.toString(), contains('test/isolate_test.dart')); } // Check that isolate is still usable @@ -467,6 +468,56 @@ void _runTests(FutureOr Function() spawner, bool terminateIsolate, ); } }); + + if (!serialize) { + test('provides complete stack traces for exceptions', () async { + // This functions have a name so that we can assert they show up in stack + // traces. + Future faultyMigration() async { + await database.customStatement('invalid syntax'); + } + + database.migration = MigrationStrategy(onCreate: (m) async { + await faultyMigration(); + }); + + try { + // The database is opened at the first statement, which will also run the + // faulty migration logic. + Future useDatabase() async { + await database.customSelect('SELECT 1').get(); + } + + await useDatabase(); + fail('Should have failed in the migration'); + } on DriftRemoteException catch (e, s) { + final trace = Chain.forTrace(s); + + // Innermost trace: The query failing on the remote isolate + expect(trace.traces, hasLength(4)); + expect( + trace.traces[0].frames[0].toString(), contains('package:sqlite3/')); + + // Then the next one: The migration being called in this isolate + expect( + trace.traces[1].frames, + contains(isA().having( + (e) => e.member, 'member', contains('faultyMigration')))); + + // This in turn is called by the server when trying to open the database. + expect( + trace.traces[2].frames, + contains(isA().having((e) => e.member, 'member', + contains('_ServerDbUser.beforeOpen')))); + + // Which, finally, happened because we were opening the database here. + expect( + trace.traces[3].frames, + contains(isA() + .having((e) => e.member, 'member', contains('useDatabase')))); + } + }); + } } DatabaseConnection _backgroundConnection() {