objectbox / objectbox-dart

Flutter database for super-fast Dart object persistence
https://docs.objectbox.io/getting-started
Apache License 2.0
927 stars 115 forks source link

ObjectBox entities with relations in isolates can't be returned to the main thread #340

Closed tneotia closed 1 year ago

tneotia commented 2 years ago

Basic info

Steps to reproduce

  1. Create a model with a relation
  2. Use an isolate to access the objects from the model box (using Flutter compute)
  3. Return those objects out of the isolate back to the main thread

Expected behavior

Dart should be able to send the class item back to the main thread.

Code

class Chat {
   //other parameters here

  final handles = ToMany<Handle>();

  @Backlink('chat')
  final messages = ToMany<Message>();

  static Future<List<Chat>> getChats({int limit = 15, int offset = 0, required Map fakeNames}) async {
     if (kIsWeb) throw Exception("Use socket to get chats on Web!");

     return await compute(getChatsIsolate, [limit, offset, prefs.getString("objectbox-reference")!, fakeNames]);
  }
}
/// Async method to get chats from objectbox
Future<List<Chat>> getChatsIsolate(List<dynamic> stuff) async {
  /// Pull args from input and create new instances of store and boxes
  store = Store.fromReference(getObjectBoxModel(), base64.decode(stuff[2]).buffer.asByteData());
  attachmentBox = store.box<Attachment>();
  chatBox = store.box<Chat>();
  handleBox = store.box<Handle>();
  messageBox = store.box<Message>();
  /// Will error right here once it tries to return the chats back
  return store.runInTransaction(TxMode.read, () {
    /// Query the [chatBox] for chats with limit and offset, prioritize pinned
    /// chats and order by latest message date
    final query = (chatBox.query()
          ..order(Chat_.isPinned, flags: Order.descending)
          ..order(Chat_.latestMessageDate, flags: Order.descending))
        .build();
    query
      ..limit = stuff[0]
      ..offset = stuff[1];
    final chats = query.find();
    query.close();

    /// Assign the handles to the chats, deduplicate, and get fake participants
    /// for redacted mode
    for (Chat c in chats) {
      c.participants = c.handles;
      c._deduplicateParticipants();
      c.fakeParticipants = c.participants.map((p) => (stuff[3][p.address] ?? "Unknown") as String).toList();
    }
    return chats;
  });
}

Logs, stack traces

Unhandled Exception: Exception: Invalid argument: "Illegal argument in isolate message : (object is a closure - Function '<anonymous closure>': static.)"
#0      Isolate._exit (dart:isolate-patch/isolate_patch.dart:650:75)
#1      Isolate.exit (dart:isolate-patch/isolate_patch.dart:653:5)
#2      _spawn (package:flutter/src/foundation/_isolates_io.dart:99:11)
<asynchronous suspension>

Additional context

We are trying to improve the performance of our already performance-heavy app, so isolate access is a must for us. Prior to trying to use Relations, we were using the SQL concept of join tables in ObjectBox, basically we had created a few separate classes to link 2 separate class IDs together, for example:

@Entity()
class ChatHandleJoin {
  int? id;
  int chatId;
  int handleId;

  ChatHandleJoin({
    required this.chatId,
    required this.handleId,
  });

  factory ChatHandleJoin.fromMap(Map<String, dynamic> map) {
    return ChatHandleJoin(
      chatId: map['chatId'] as int,
      handleId: map['handleId'] as int,
    );
  }
}

Relations seem like a much better way of accomplishing this, and greatly reduces some of the code we need on functions that deal with lists of these entities. However, this issue where the isolate cannot return a class with relations is deal breaking, is there any solution or workaround to get this behavior to work properly?

tneotia commented 2 years ago

I just tried the same behavior with Isolate.spawn on Flutter 2.8 beta, and this time it gives a different error:

Invalid argument(s): Illegal argument in isolate message: (object is aPointer)
_SendPortImpl._sendInternal (dart:isolate-patch/isolate_patch.dart:249:43)
_SendPortImpl.send (dart:isolate-patch/isolate_patch.dart:230:5)
getChatsIsolate (package:bluebubbles/repository/models/io/chat.dart:242:17)
_delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:300:17)
_RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:192:12)

I'm guessing this is because ToMany and ToOne have references to the box and store inside of them using Pointer, which is explicitly not able to be sent through SendPort.send. I don't know if this can be changed / fixed on the ObjectBox side in any way, but if there's a workaround I can use, it would be great!

Edit: I did some experimenting & research and found the async_task package. That has allowed me to "convert" the synchronous ObjectBox methods into an asynchronous task, which is also fine from a performance perspective, it only feels slightly worse than using isolates (but then again, it is also slightly faster).

Here's what the code looks like now:

/// Async method to get chats from objectbox
class GetChats extends AsyncTask<List<dynamic>, List<Chat>> {
  final List<dynamic> stuff;

  GetChats(this.stuff);

  @override
  AsyncTask<List<dynamic>, List<Chat>> instantiate(List<dynamic> parameters, [Map<String, SharedData>? sharedData]) {
    return GetChats(parameters);
  }

  @override
  List<dynamic> parameters() {
    return stuff;
  }

  @override
  FutureOr<List<Chat>> run() {
    return store.runInTransaction(TxMode.read, () {
      /// Query the [chatBox] for chats with limit and offset, prioritize pinned
      /// chats and order by latest message date
      final query = (chatBox.query()
        ..order(Chat_.isPinned, flags: Order.descending)
        ..order(Chat_.latestMessageDate, flags: Order.descending))
          .build();
      query
        ..limit = stuff[0]
        ..offset = stuff[1];
      final chats = query.find();
      query.close();

      /// Assign the handles to the chats, deduplicate, and get fake participants
      /// for redacted mode
      for (Chat c in chats) {
        c.participants = List<Handle>.from(c.handles);
        c._deduplicateParticipants();
        c.fakeParticipants = c.participants.map((p) => (stuff[3][p.address] ?? "Unknown") as String).toList();
      }
      return chats;
    });
  }
}
Future<List<Attachment>> getAttachmentsAsync() async {
    if (kIsWeb || id == null) return [];

    final task = GetChatAttachments([id!, prefs.getString("objectbox-reference")]);
    return (await createAsyncTask<List<Attachment>>(task)) ?? [];
}
/// Create a "fake" asynchronous task from a traditionally synchronous task
///
/// Used for heavy ObjectBox read/writes to avoid causing jank
Future<T?> createAsyncTask<T>(AsyncTask<List<dynamic>, T> task) async {
  final executor = AsyncExecutor(parallelism: 0, taskTypeRegister: () => [task]);
  executor.logger.enabled = true;
  executor.logger.enabledExecution = true;
  await executor.execute(task);
  return task.result;
}

ObjectBox devs, if fixing the Pointer issue feels out of scope, feel free to close this issue. The AsyncTask workaround is enough for me :). And finally, thanks for such an excellent DB - it has really revolutionized the performance and speed of our app compared to sqflite!

greenrobot-team commented 2 years ago

Thanks for reporting, it looks like you already found a good solution using the async_task package.

I have limited knowledge about Isolates, but looking at the compute docs states that sending object instances is only supported for Dart Native. Or could you already get this working without the relation fields?

Also I wonder why async_task works, since it claims to use Isolates as well? Edit: I looked at docs and source code, and it appears it has the same limitations of what can be sent. So I'm not sure how this is working in your case.

Anyhow, current Isolate tests in ObjectBox are limited to sending the Store.reference to the Isolate, but not actually sending full objects back. See https://github.com/objectbox/objectbox-dart/blob/main/objectbox/test/isolates_test.dart

Edit: To come back to the Store reference issue of ToMany and ToOne: this should only be a problem once put has been called on an object. Only then is relation info set on them (e.g. Store and Box references).

tneotia commented 2 years ago

I have limited knowledge about Isolates, but looking at the compute docs states that sending object instances is only supported for Dart Native. Or could you already get this working without the relation fields?

I had this working already without the relation fields. As of recent versions of Dart (I think 2.14 maybe?), you can now send almost anything between isolates using SendPort.send, as long as the isolates share the same code, which is the case for isolates created via Isolate.spawn. The docs state the only types that cannot be sent are "Objects with native resources", ReceivePort, DynamicLibrary, Pointer, UserTag, and MirrorReference, which is where the Pointer issue arises for relation fields.

Also I wonder why async_task works, since it claims to use Isolates as well?

The package only uses isolates when you set parallelism >= 1, and in my case I set it to 0. This means that rather than using isolates it uses zone.microtask to make a "fake" async task, and thus everything happens on the same thread. If I had parallelism: 1, I would've run into the same issue.

Anyhow, current Isolate tests in ObjectBox are limited to sending the Store.reference to the Isolate, but not actually sending full objects back.

Yep I noticed this as well but I decided to experiment and see if I could send stuff back to the main isolate. To be honest, I think having an async box get / put API would be very nice, as right now everything is synchronous. On mobile especially this can cause microstutters and janked frames - even Flutter themselves recommend doing File IO asynchronously.

To come back to the Store reference issue of ToMany and ToOne: this should only be a problem once put has been called on an object.

I did try a trick of setting these to null right before passing them through SendPort.send and then setting them back to the ToMany and ToOne relation on the main thread side, but I still got the same error unfortunately. I think its just because of how the parameter itself is typed - Dart knows its supposed to be a ToMany, which contains Box, which contains Pointer and that's not allowed.

And just an update on my async_task method - while its definitely better than having everything be synchronous, it is not as performant as using a separate isolate and making the database calls on a different thread. We can live with the slight performance drop, but in the long term if a solution can be found for this issue it would be great :). Also really looking forward to Web support with Indexed DB!

greenrobot-team commented 2 years ago

Thanks for all those additional details!

you can now send almost anything between isolates

Looking at https://api.dart.dev/stable/2.14.4/dart-isolate/SendPort/send.html it still says what I wrote. Am I looking at the wrong docs?

Anyhow, could reproduce with https://github.com/objectbox/objectbox-dart/compare/isolates-send-objects. With Flutter 2.2.3 there is even a useful error message:

Invalid argument(s): Native objects (from dart:ffi) such as Pointers and Structs cannot be passed between isolates.
#0      _SendPortImpl._sendInternal (dart:isolate-patch/isolate_patch.dart:235:70)
#1      _SendPortImpl.send (dart:isolate-patch/isolate_patch.dart:219:5)
#2      createDataIsolate (/objectbox-dart/objectbox/test/isolates_test.dart:155:22)

Which seems to confirm your suspicion that Dart doesn't care if a Pointer is actually not null, but only the presence of the type is an issue. That might be a problem for a fix given how ToMany and ToOne and the attach-logic currently work. Will look into this more later.

tneotia commented 2 years ago

Looking at https://api.dart.dev/stable/2.14.4/dart-isolate/SendPort/send.html it still says what I wrote. Am I looking at the wrong docs?

My apologies, the beta docs have it: https://api.dart.dev/beta/2.15.0-268.18.beta/dart-isolate/SendPort/send.html. We built our app on Flutter 2.5 stable, which shipped with Dart 2.14, and the isolate communication was still working fwiw, so I guess they didn't update the stable docs quite yet.

Will look into this more later.

Thank you guys so much!

greenrobot-team commented 1 year ago

Just to update this: it is currently not possible to return objects that contain ToOne and ToMany from an isolate if the ToOne and ToMany are attached (e.g. they are returned from a Box method like get or have been put). This also affects the new runAsync and runInTransactionAsync functions.

However, it is possible to send objects containing ToOne/ToMany that are not attached. These are typically newly created objects.

It's possible that when using a closure as the callback for an isolate sending still fails, but this is then due to a Dart bug that over-captures surrounding state (e.g. you have a Store variable somewhere that is captured and sent).

We hope to resolve this in a future release.

tneotia commented 1 year ago

Just wanted to bump this - would it be feasible to "detach" the ToOne / ToMany objects when running an async transaction? E.g. you could add a parameter to runAsync for ignoreRelations: true. Then maybe provide some API to re-attach relations, like <object>.attachRelations() or List<object>.attachRelationsMany(), which would be called on the objects received from the runAsync function.

I don't know exactly how relations function under the hood, but if attaching them doesn't require running any I/O operations it could still mean a significant performance boost in using isolates over an async zone callback.

greenrobot-team commented 1 year ago

@tneotia Thanks for the suggestions! We already have plans for this internally, just haven't gotten around to implement them, yet.

tneotia commented 1 year ago

Thanks for the update! I'd really love to see this in the near future - we're working on a full rewrite of our app's UI code in the hopes of improving performance, and this feature would help us significantly towards that end.

greenrobot-team commented 1 year ago

@tneotia We have released preview version 1.8.0-dev.0 which allows to send objects containing ToOne and ToMany relations across isolates, e.g. when using store.runInTransactionAsync. We would welcome any feedback and additional testing on this.

(Note: using flutter pub upgrade will not update to this preview version, manually change the version in your pubspec.yaml instead and run flutter pub get.)

greenrobot-team commented 1 year ago

This is now possible with the new 2.0.0 release.