objectbox-dart icon indicating copy to clipboard operation
objectbox-dart copied to clipboard

[Flutter] query.watch() never seems to close the underlying query

Open navaronbracke opened this issue 3 years ago • 3 comments

When using query(condition).watch() you get a Stream of changes. However, there does not seem to be a way to close the underlying query.

As stated in the docs for Query, we should close the query when we are done with it.

Call [Query.close()] after you're done with it to free resources.

However the underlying StreamController only calls subscription.cancel() and not query.close() when it is closed.

Basic info (please complete the following information):

  • ObjectBox version: 1.6.0
  • Flutter/Dart SDK: Flutter 3.0.2 / Dart 2.17.3
  • Null-safety enabled: yes
  • Reproducibility: always
  • OS: MacOS 12.2.1
  • Device/Emulator: N/A

Additionally, you can choose to provide more details, e.g. the output of:

  • flutter doctor -v
[✓] Flutter (Channel stable, 3.0.2, on macOS 12.2.1 21D62 darwin-x64, locale
    en-BE)
    • Flutter version 3.0.2 at /Users/navaronbracke/Documents/flutter
    • Upstream repository [email protected]:flutter/flutter.git
    • Framework revision cd41fdd495 (7 weeks ago), 2022-06-08 09:52:13 -0700
    • Engine revision f15f824b57
    • Dart version 2.17.3
    • DevTools version 2.12.2

[✓] Android toolchain - develop for Android devices (Android SDK version 33.0.0)
    • Android SDK at /Users/navaronbracke/Library/Android/sdk
    • Platform android-33, build-tools 33.0.0
    • ANDROID_HOME = /Users/navaronbracke/Library/Android/sdk
    • Java binary at: /Applications/Android
      Studio.app/Contents/jre/Contents/Home/bin/java
    • Java version OpenJDK Runtime Environment (build
      11.0.12+0-b1504.28-7817840)
    • All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS (Xcode 13.4.1)
    • Xcode at /Applications/Xcode.app/Contents/Developer
    • CocoaPods version 1.11.2

[✓] Chrome - develop for the web
    • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome

[✓] Android Studio (version 2021.2)
    • Android Studio at /Applications/Android Studio.app/Contents
    • Flutter plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/9212-flutter
    • Dart plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/6351-dart
    • Java version OpenJDK Runtime Environment (build
      11.0.12+0-b1504.28-7817840)

[✓] VS Code (version 1.69.2)
    • VS Code at /Applications/Visual Studio Code.app/Contents
    • Flutter extension version 3.44.0

[✓] Connected device (2 available)
    • macOS (desktop) • macos  • darwin-x64     • macOS 12.2.1 21D62 darwin-x64
    • Chrome (web)    • chrome • web-javascript • Google Chrome 103.0.5060.134

[✓] HTTP Host Availability
    • All required HTTP hosts are available

• No issues found!

Steps to reproduce

  1. Use store.box<T>().query(condition).watch() to create a Stream
  2. Setup a StreamSubscription for the stream
  3. Cancel the subscription
  4. The query never called its close() method (I can only speculate here)

Expected behavior

The underlying query should be disposed when the subscription is cancelled.

Code

  Stream<Query<T>> watch({bool triggerImmediately = false}) {
    final queriedEntities = HashSet<Type>();
    _fillQueriedEntities(queriedEntities);
    final query = build();
    late StreamSubscription<void> subscription;
    late StreamController<Query<T>> controller;
    final subscribe = () {
      subscription = _store.entityChanges.listen((List<Type> entityTypes) {
        if (entityTypes.any(queriedEntities.contains)) {
          controller.add(query);
        }
      });
    };
    controller = StreamController<Query<T>>(
        onListen: subscribe,
        onResume: subscribe,
        onPause: () => subscription.pause(),
        onCancel: () => subscription.cancel()); // The query should also be closed here?
    if (triggerImmediately) controller.add(query);
    return controller.stream;
  }

I'd expect to see

onCancel: (){
  subscription.cancel();
  query.close();
}

Logs, stack traces

N/A

Additional context

N/A

navaronbracke avatar Jul 26 '22 14:07 navaronbracke

Thanks for reporting! Yes, we should probably change this.

Note, the native query resources are still getting cleaned up because Query has a finalizer attached. So once the subscription is garbage collected, the query is currently closed as well. Still, it probably should close the query immediately when the stream subscription is canceled.

greenrobot-team avatar Aug 01 '22 08:08 greenrobot-team

Hm, looks like this is on purpose to support a use case where calling code might keep the returned query instance: https://github.com/objectbox/objectbox-dart/blob/362a4ddcbc671e9efb2e21fb298361f2f4d60da2/objectbox/test/stream_test.dart#L94-L105

So not sure if explicitly closing is a good idea then, might break existing code.

greenrobot-team avatar Aug 01 '22 08:08 greenrobot-team

Ok, so the native query has a finalizer and will probably get GC'ed when the last of its subscribers cancels its subscription.

So what if you return the StreamController when using query.watch() ? Then a client can cancel the controller while the query lives on until its finalizer is called. This would be a breaking change

  • clients would need to dispose of the controller
  • clients would need to use controller.stream to get the stream

navaronbracke avatar Aug 01 '22 08:08 navaronbracke