Monitoring a characteristic and then cancelling has a race condition

[cbreezier]

When using Peripheral.monitorCharacteristic(...) we get a Stream.

When we cancel that stream, the monitoring should be fully cancelled (ie, native resources cleaned up) after we await on the cancelling of the stream. However, the actual cleanup of resources is performed asynchronously.

    final notification = device.monitorCharacteristic(
      SERVICE,
      CHARACTERISTIC,
    )
        .first; // This turns the Stream into a Future. Upon completion of the future, the stream subscription is canceled.

    final result = await notification;

    print('after cancel');

Log output:

D/com.polidea.flutter_ble_lib.FlutterBleLibPlugin(29627): on native side observed method: monitorCharacteristicForDevice
D/BluetoothGatt(29627): setCharacteristicNotification() - uuid: 0000ac49-53a6-4f72-96c8-fd6e6964bae3 enable: true
I/flutter (29627): after cancel
D/com.polidea.flutter_ble_lib.FlutterBleLibPlugin(29627): on native side observed method: cancelTransaction

Note that the after cancel message is logged before the cancelTransaction was observed, whereas it should be logged after.

---

The root cause is that the stream returned from monitorCharacteristic is a broadcast stream, not a single subscription stream.

characteristics_mixin.dart - _createMonitoringStream:

    StreamController<CharacteristicWithValue> streamController =
        StreamController.broadcast(
      onListen: onListen,
      onCancel: () => cancelTransaction(transactionId),
    );

This means that the onCancel callback does not return a Future - it just asynchronously tries to clean up resources.

I believe that the monitoring stream should be a single subscription stream. If consumers wish to have multiple subscribers, they can always convert the single subscription stream into a broadcast stream in their application logic.

[mikolak]

Woah, that's some thorough investigation! We can leverage Dart's dependency system and do a quick check if that works - could you edit that file in your local copy and delete .broadcast in _createMonitoringStream(...)?

This is a potentially breaking change, so I'll have to figure out how to approach it.

[cbreezier]

Sorry for the late reply - I tried editing my local copy (forgot you could do that!) and I can confirm that after removing the .broadcast I now reliably observe the after cancel log message after the on native side observed method: cancelTransaction message.

I agree that it's a breaking change - I suspect that most consumers won't be affected, but that's not really good enough :P

Perhaps an optional boolean parameter broadcastStream which defaults to true to maintain backwards compatibility, but recommend setting it to false in the documentation to avoid cleanup issues? I'm sure you'll figure something out :)

[dariuszseweryn]

Could you describe a scenario under which the current approach falls short? I mean besides the principal rule that unsubscribing should be awaitable.

I might be asking a newbie question – just getting started with Dart and Flutter.

[cbreezier]

@dariuszseweryn due to the way that my app is structured, and the nature of the ble devices that I'm interacting with, I often need to temporarily monitor and then stop monitoring certain characteristics - and often the same characteristic.

Since I can't stop monitoring a characteristic reliably, and the bluetooth stack throws an error when you try and monitor a characteristic that is already monitored, that means that I only have two main options (that I can see):

• restructure my app to keep monitoring a characteristic indefinitely, and build another layer of abstraction above the actual monitoring to "stop monitoring", or
• add a hacky timeout after canceling a monitoring request (I've found 1 second to unreliably work, and 3 seconds to reliably work)

Both are a little clunky, and the second option makes for a bad user experience, although it is much easier to implement.

All would be solved if I could just reliably wait for the monitoring to be stopped and cleaned up :)

[dariuszseweryn]

the bluetooth stack throws an error when you try and monitor a characteristic that is already monitored

I am not aware of this. Could you show an example of such error?

[cbreezier]

@dariuszseweryn sorry, that part was largely guesswork on my part - I haven't actually dived deep enough to verify that the exact problem is monitoring an already-monitored characteristic.

The error I get at the flutter level is a BleError with error code 2, whatever that means. It happens whenever I try and monitor a characteristic, but the previous monitoring stream hasn't been cleaned up yet.

When I modify the broadcast stream to a single-subscription stream, and allow the previous monitoring stream to complete its cleanup properly, I no longer get any of these errors.

A little circumstantial, but it seemed like strong enough evidence that monitoring an already-monitored characteristic was the cause for the error.

For context, this is on a Pixel 3 running Android 10.

[mikolak]

2 is operation cancelled, which is called automatically in onCancel of the stream.

Single stream makes sense I think.

[chandong83]

Hello, I have the same problem. Does the current solution have to edit the code directly for the edited by @cbreezier? Thank you.

[cbreezier]

@mikolak Hi, I'm just coming back to look at this again.

The fix for this issue is quite simple (move to a single-subscription stream), and the reason why it hasn't been merged (which I completely understand) is because it's a potentially breaking change.

However, since Flutter 2.0 and the big migration to null-safety, I think it is a great opportunity to introduce breaking changes where necessary. I've seen other libraries take this opportunity (eg Dio) with a major version bump and some breaking changes - could we do that as well?

For what it's worth, I've been running with this change on a fork for the last few months, and released a production app with it which relies on monitoring characteristics quite heavily with no issue.

[mikolak]

It absolutely makes sense. I think that 3.0.0 (which is now exposed as beta with MBA migrated to RxJava 2) is a great candidate for this, the migration and null safety, possibly few others, and I'd like to release it with all those. My time for the library has shrinked considerably, so it might take a while.