Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion lib/flutter_midi_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,13 @@ class MidiCommand {
timeout: awaitConnectionTimeout,
);
final connectionEstablished = _awaitConnectedOrFailed(device);
// A disconnection can complete this future with an error before the real
// handler below is attached (e.g. a disconnect racing an in-flight connect).
// Guard it eagerly so the race never surfaces as an unhandled async error
// to PlatformDispatcher.onError (issue #160). Dart futures allow multiple
// independent listeners, so the `await` below still observes and handles
// the same error inside its try/catch.
unawaited(connectionEstablished.catchError((_) {}));

try {
final route = _resolveDeviceRoute(device);
Expand Down Expand Up @@ -399,7 +406,6 @@ class MidiCommand {
);
}
} catch (_) {
unawaited(connectionEstablished.catchError((_) {}));
_cancelBlePlatformHandoff(device.id);
_activeDeviceRouteById.remove(device.id);
if (device.connectionState != MidiConnectionState.disconnected) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,18 @@ import com.invisiblewrench.fluttermidicommand.pigeon.MidiHostDevice
import com.invisiblewrench.fluttermidicommand.pigeon.MidiPacket
import com.invisiblewrench.fluttermidicommand.pigeon.MidiSetupChange

/**
* Runs each teardown [actions] in order, swallowing any error, so a failure in
* one step (for example an `IOException: EPIPE` from a port whose device was
* unplugged) cannot abort the remaining steps or the disconnection
* notifications that follow. Never throws.
*/
internal fun runTeardownQuietly(vararg actions: () -> Unit) {
for (action in actions) {
runCatching { action() }
}
}

class ConnectedDevice(
device: MidiDevice,
private val logicalDeviceId: String,
Expand Down Expand Up @@ -71,12 +83,20 @@ class ConnectedDevice(
}

override fun close() {
this.inputPort?.flush()
this.inputPort?.close()
this.outputPort?.close()
this.outputPort?.disconnect(this.receiver)
// When a device is physically removed (e.g. USB unplugged) its file
// descriptor is already gone, so any teardown call can throw
// (IOException: EPIPE). runTeardownQuietly never propagates, so one
// failing step cannot abort the others or escape the framework's
// onDeviceRemoved callback, and the disconnection notifications below
// always fire.
runTeardownQuietly(
{ this.inputPort?.flush() },
{ this.inputPort?.close() },
{ this.outputPort?.close() },
{ this.outputPort?.disconnect(this.receiver) },
{ this.midiDevice.close() },
)
this.receiver = null
this.midiDevice.close()

onSetupChanged(MidiSetupChange.DEVICE_DISCONNECTED)
onConnectionChanged(id, false)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
package com.invisiblewrench.fluttermidicommand

import java.io.IOException
import kotlin.test.Test
import kotlin.test.assertEquals

/**
* Regression tests for issue #158: unplugging a connected USB MIDI device
* crashed the app because `ConnectedDevice.close()` called `inputPort.flush()`
* on a dead file descriptor and the resulting `IOException: EPIPE` aborted the
* rest of teardown and the disconnection notifications.
*
* `close()` routes every fragile Android teardown call through
* [runTeardownQuietly] and then unconditionally fires the notifications, so
* these tests lock in the two guarantees that keep the crash fixed: a throwing
* step never aborts the later steps, and the helper never propagates.
*/
class ConnectedDeviceCloseTest {

@Test
fun runsEveryStepEvenWhenSomeThrow() {
val ran = mutableListOf<String>()

runTeardownQuietly(
{
ran.add("flush")
throw IOException("write failed: EPIPE (Broken pipe)")
},
{ ran.add("closeInput") },
{
ran.add("closeOutput")
throw IllegalStateException("already closed")
},
{ ran.add("closeDevice") },
)

assertEquals(
listOf("flush", "closeInput", "closeOutput", "closeDevice"),
ran,
)
}

@Test
fun neverPropagates() {
// If this threw, the test would fail; passing proves the EPIPE is
// swallowed so the notifications after the teardown block in close()
// always run.
runTeardownQuietly(
{ throw IOException("write failed: EPIPE (Broken pipe)") },
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,14 @@ const midiCharacteristicId = "7772E5DB-3868-4112-A1A9-F2669D106BF3";

enum _DeviceState { none, interrogating, available, irrelevant }

/// True when [error] is the universal_ble error surfaced when a peripheral has
/// discarded its side of a previous bond (iOS `CBErrorPeerRemovedPairingInformation`,
/// "Peer removed pairing information"). It arrives as an untyped
/// `UniversalBleException` with `unknownError`, so match on the message.
bool _isPairingInfoRemoved(Object error) =>
error is UniversalBleException &&
error.message.toLowerCase().contains('pairing information');

enum _BleHandlerState {
header,
timestamp,
Expand Down Expand Up @@ -396,11 +404,23 @@ class _BleMidiDevice extends MidiDevice {
}
await _prepareMidiReadiness(timeout: timeout);
connected = true;
} catch (_) {
} catch (error) {
connected = false;
try {
await disconnect();
} catch (_) {}
if (_isPairingInfoRemoved(error)) {
// Best-effort clear of the stale bond so a later reconnect can re-pair
// cleanly. Unsupported on iOS (CoreBluetooth has no unpair API), so
// ignore failures; the surfaced exception tells the user what to do.
try {
await UniversalBle.unpair(deviceId);
} catch (_) {}
throw MidiPairingInfoRemovedException(
deviceId: deviceId,
cause: error,
);
}
rethrow;
} finally {
_readinessInProgress = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import 'package:universal_ble/universal_ble.dart';
class _FakeUniversalBlePlatform extends UniversalBlePlatform {
AvailabilityState availabilityState = AvailabilityState.poweredOn;
final Set<String> failingConnectIds = <String>{};
final Set<String> pairingRemovedConnectIds = <String>{};
final Set<String> failingReadIds = <String>{};
final Set<String> failingSubscribeIds = <String>{};
final Set<String> rejectedPairIds = <String>{};
Expand All @@ -20,6 +21,7 @@ class _FakeUniversalBlePlatform extends UniversalBlePlatform {
final List<String> connectCalls = <String>[];
final List<String> disconnectCalls = <String>[];
final List<String> pairCalls = <String>[];
final List<String> unpairCalls = <String>[];
final List<String> readCalls = <String>[];
final List<String> subscribeCalls = <String>[];
int startScanCalls = 0;
Expand Down Expand Up @@ -65,6 +67,14 @@ class _FakeUniversalBlePlatform extends UniversalBlePlatform {
}) async {
connectCalls.add(deviceId);
await Future<void>.delayed(const Duration(milliseconds: 1));
if (pairingRemovedConnectIds.contains(deviceId)) {
updateConnection(deviceId, false, 'Peer removed pairing information');
_connectionByDevice[deviceId] = BleConnectionState.disconnected;
throw UniversalBleException(
code: UniversalBleErrorCode.unknownError,
message: 'Peer removed pairing information',
);
}
if (failingConnectIds.contains(deviceId)) {
updateConnection(deviceId, false, 'connection-failed');
_connectionByDevice[deviceId] = BleConnectionState.disconnected;
Expand Down Expand Up @@ -159,6 +169,7 @@ class _FakeUniversalBlePlatform extends UniversalBlePlatform {

@override
Future<void> unpair(String deviceId) async {
unpairCalls.add(deviceId);
_pairedByDevice[deviceId] = false;
updatePairingState(deviceId, false);
}
Expand Down Expand Up @@ -319,6 +330,30 @@ void main() {
expect(await transport.devices, isEmpty);
});

test(
'connectToDevice maps removed pairing information to a typed exception',
() async {
fakePlatform.servicesByDevice['ble-stale-bond'] = midiServices();
fakePlatform.pairingRemovedConnectIds.add('ble-stale-bond');
fakePlatform.emitScanDevice(
BleDevice(
deviceId: 'ble-stale-bond',
name: 'Stale Bond Device',
services: <String>[],
),
);
final device = (await transport.devices).single;

await expectLater(
transport.connectToDevice(device),
throwsA(isA<MidiPairingInfoRemovedException>()),
);
expect(device.connected, isFalse);
// The stale bond is cleared best-effort so a later reconnect re-pairs.
expect(fakePlatform.unpairCalls, contains('ble-stale-bond'));
},
);

test('disconnectDevice forwards to BLE backend', () async {
fakePlatform.servicesByDevice['ble-3'] = midiServices();
fakePlatform.emitScanDevice(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,15 @@ class MethodChannelMidiCommand extends MidiCommandPlatform
);
device.name = hostDevice.name ?? '-';
device.type = _fromHostDeviceType(hostDevice.type);
device.connected = hostDevice.connected ?? false;
// A passive device-list refresh must not collapse a transitional state. The
// `connected` setter can only express connected/disconnected, so applying it
// while a connect/disconnect is in flight would clobber the connecting/
// disconnecting state those explicit flows (and their native callbacks) own.
final state = device.connectionState;
if (state != MidiConnectionState.connecting &&
state != MidiConnectionState.disconnecting) {
device.connected = hostDevice.connected ?? false;
}
device.inputPorts = _fromHostPorts(hostDevice.inputs, MidiPortType.IN);
device.outputPorts = _fromHostPorts(hostDevice.outputs, MidiPortType.OUT);
if (id.isNotEmpty) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,16 @@ class MidiPairingFailedException extends MidiConnectionException {
: super(stage: MidiConnectionStage.pairing, message: 'Pairing failed.');
}

class MidiPairingInfoRemovedException extends MidiConnectionException {
MidiPairingInfoRemovedException({required super.deviceId, super.cause})
: super(
stage: MidiConnectionStage.pairing,
message:
'The device removed its pairing information. Forget the device in '
'the system Bluetooth settings, then pair again.',
);
}

class MidiServiceDiscoveryException extends MidiConnectionException {
MidiServiceDiscoveryException({required super.deviceId, super.cause})
: super(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,34 @@ void main() {
},
);

test(
'devices does not clobber the connecting state of an in-progress device',
() async {
// The host still reports connected:false while a connect is in flight.
final host =
_FakeHostApi()
..listedDevices = <pigeon.MidiHostDevice>[
pigeon.MidiHostDevice(
id: 'serial-1',
name: 'Serial',
type: pigeon.MidiDeviceType.serial,
connected: false,
),
];
final platform = MethodChannelMidiCommand(hostApi: host);

final device = (await platform.devices)!.single;
await platform.connectToDevice(device);
expect(device.connectionState, MidiConnectionState.connecting);

// A concurrent device-list refresh must reuse the same instance without
// collapsing the transitional state back to disconnected (issue #159).
final refreshed = (await platform.devices)!.single;
expect(identical(refreshed, device), isTrue);
expect(device.connectionState, MidiConnectionState.connecting);
},
);

test(
'disconnect triggers host disconnect and transitions to disconnected',
() async {
Expand Down
57 changes: 57 additions & 0 deletions test/flutter_midi_command_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,19 @@ class _FailedConnectPlatform extends _FakePlatform {
}
}

class _HangingConnectPlatform extends _FakePlatform {
@override
Future<void> connectToDevice(
MidiDevice device, {
List<MidiPort>? ports,
}) async {
connected.add(device.id);
// Stay in flight without ever marking the device connected, so a
// disconnection can race the connect while it is still pending.
await Future<void>.delayed(const Duration(milliseconds: 20));
}
}

class _FakePlatformWithBleHost extends _FakePlatform {
@override
Future<List<MidiDevice>?> get devices async => [
Expand Down Expand Up @@ -501,6 +514,50 @@ void main() {
);
});

test(
'disconnecting while connecting does not raise an unhandled async error',
() async {
final uncaught = <Object>[];
await runZonedGuarded(
() async {
final platform = _HangingConnectPlatform();
MidiCommand.setPlatformOverride(platform);
final midi = MidiCommand();

final device = (await midi.devices)!.first;

Object? callerError;
final future = midi
.connectToDevice(
device,
awaitConnectionTimeout: const Duration(seconds: 1),
)
.catchError((Object error) {
callerError = error;
});

// Race a disconnection against the still-in-flight connect.
device.setConnectionState(MidiConnectionState.disconnected);

await future;

// The caller's handler receives the failure...
expect(callerError, isA<StateError>());
},
(error, _) {
uncaught.add(error);
},
);

// ...and nothing leaks to the zone's uncaught-error handler (issue #160).
expect(
uncaught,
isEmpty,
reason: 'connect/disconnect race must not surface an unhandled error',
);
},
);

test('dispose releases singleton and allows creating a new instance', () {
final platform = _FakePlatform();
MidiCommand.setPlatformOverride(platform);
Expand Down