Query stream issue #151 #152
Conversation
|
@greenrobot @Buggaboo @vaind could you check please? |
|
And almost forgotten... is it possible to test this? Would a small test case to verify the issue was there before (and is not anymore after the changes) be too hard to do? That'd help make avoid future regressions. |
test(
'Only observers of a single entity are notified, no cross-entity observer notification',
() async {
// setup listeners
final box2 = Box<TestEntity2>(env.store);
var counter1 = 0, counter2 = 0;
final query2 = box2.query().build();
final queryStream2 = query2.findStream();
final subscription2 = queryStream2.listen((_) {
counter2++;
});
final query1 = box.query().build();
final queryStream1 = query1.findStream();
final subscription1 = queryStream1.listen((_) {
counter1++;
});
// counter2 test #.1
final t2 = TestEntity2();
box2.put(t2);
await Future.delayed(Duration(seconds: 0));
expect(counter1, 0);
expect(counter2, 1);
// counter1 test #.1
final t1 = TestEntity();
box.put(t1);
await Future.delayed(Duration(seconds: 0));
expect(counter1, 1);
expect(counter2, 1);
// counter1 many test #.2
final ts1 = [1, 2, 3].map((i) => TestEntity(tInt: i)).toList();
box.putMany(ts1);
await Future.delayed(Duration(seconds: 0));
expect(counter1, 2);
expect(counter2, 1);
// counter2 many test #.2
final ts2 = [1, 2, 3].map((i) => TestEntity2()).toList();
box2.putMany(ts2);
await Future.delayed(Duration(seconds: 0));
expect(counter1, 2);
expect(counter2, 2);
query1.close();
query2.close();
await subscription1.cancel();
await subscription2.cancel();
});
// TODO query.stream testI'm looking into it. |
@vaind I wanted to add tests but I have problems running on mac OS |
|
Have you run 'install.sh'? Also stripped the 'dart' exec? |
lib/src/observable.dart
Outdated
| @@ -9,7 +9,7 @@ import 'util.dart'; | |||
| // ignore_for_file: non_constant_identifier_names | |||
|
|
|||
| // dart callback signature | |||
| typedef Any = void Function(Pointer<Void>, Pointer<Uint32>, int); | |||
There was a problem hiding this comment.
The change to the 2nd param is questionable.
The array should be unpacked, and the entityId it contains should be used to trigger the specific callback(s) indexed by storePtr and entityId. I might have been a bit sloppy.
There was a problem hiding this comment.
The change to the 2nd param is questionable.
The array should be unpacked, and the entityId it contains should be used to trigger the specific callback(s) indexed by storePtr and entityId. I might have been a bit sloppy.
@Buggaboo I didn't change it. Please look at the end result.
install.sh ? |
Thank you i resolved it |
|
Nope. Improper testing on my side. |
He is behaving correctly. The problem is that everyone subscribes to a single stream controller, which notifies everyone. |
|
@Buggaboo In my opinion, the best solution is to move this |
|
@vaind Do we still have to close Queries? Or did I forgot to add them? |
|
I found another (conceptual) bug. Fixing it. The number of C callback notifications should not increase linearly along with the number of dart listeners. Only one C observer per store should be initialized to serve notifications. |
|
I took your changes with a fix for another bug I found. I have to redo it again because I based off the wrong branch. |
I was looking at an old branch :(, that other problem was fixed already. |
What problem was fixed? |
Only one C observer per store should be initialized to serve notifications. |
|
|
|
@vaind could do it. I don't have the permissions. @RTrackerDev he's a busy guy. Regardless, your contribution is appreciated. (At least by me) |
|
thx for the fix @RTrackerDev and the support @Buggaboo, merging. FYI there will be major changes to query streams implementation, to support asynchronous callbacks (across multiple isolates), see #145. |
Minor fix for #151