-
Notifications
You must be signed in to change notification settings - Fork 135
Data visitor #95
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Data visitor #95
Conversation
import "package:ffi/ffi.dart" show allocate, free; | ||
|
||
int _lastId = 0; | ||
final _callbacks = <int, bool Function(Pointer<Uint8> dataPtr, int length)>{}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do we need that for? E.g. this is one of the visitor C APIs:
obx_err obx_query_visit(OBX_query* query, obx_data_visitor* visitor, void* user_data, uint64_t offset, uint64_t limit);
I assume you want to pass "id" as user_data
? Is this more "convenient" that passing in a lambda-like thingy as visitor
? The latter would already be thread-safe, which the current map-based is not, I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lambdas can't be passed as a C callback, FFI requires static functions. I've tried that (lambda) approach but had to resolve to this trampoline one instead (basically the same thing we do in Go)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, please make sure to document that in the code so that this explanation is not lost
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latter would already be thread-safe, which the current map-based is not, I think?
I think this was not addressed yet. Does that require additional thread synchronization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no concurrent memory access in Dart. All code in a single isolate executes on a single event-loop and isolates don't share memory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how so? I don't see a problem with the code in this PR. You mean something unrelated, e.g. transactions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To my limited understanding, I imagine this to happen:
- Let's assume two isolates, both starting with IDs at 1 (produce colliding IDs, because isolated...)
- Callbacks from core happen on a different thread
- (Open question: not sure about how a "foreign" thread is mapped to an isolate. So, which isolate gets the callback?)
- Now, I don't see how we ensured that one isolate gets only its callback in using its ID.
E.g. Isolate 2 registers a callback for ID 1, but isolate 1 gets to consume it because it also has a pending callback for ID 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Callbacks from core happen on a different thread
They don't with objectbox-c visitors - all calls are synchronous, executed on the same thread. This approach wouldn't work for other kinds of callbacks, where you register the callback and it may happen in the future (e.g. some kind of event listeners). Not sure how that could be handled with dart, if at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it's an open issue with some workarounds available dart-lang/sdk#37022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that one is strictly synchronous and if FFI guarantees synchronous callbacks to execute in the same isolate (I assume that is defined somewhere?), we should be fine.
👍 From my brief look it's good to merge after those two tiny things are addressed. |
Fixes #94