Skip to content

Conversation

@sgrekhov
Copy link
Contributor

No description provided.

@lrhn
Copy link
Member

lrhn commented Oct 15, 2024

Are there tests for the MultiStreamController API? Which is the normal StreamController API plus addSync, addErrorSync and closeSync, which allows adding events synchronously (after listen has returned, not paused, and if there are no prior events still enqueued).

Copy link
Contributor Author

@sgrekhov sgrekhov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. PTAL.

Are there tests for the MultiStreamController API? Which is the normal StreamController API plus addSync, addErrorSync and closeSync, which allows adding events synchronously (after listen has returned, not paused, and if there are no prior events still enqueued).

I'm working on these tests now and will add them later in a separate PR.

BTW could you, please, help me to understand if the following test is correct or not?

main() {
  asyncStart(2);
  int counter = 1;
  var stream = Stream<int>.multi((controller) {
    for (;;counter++) {
      controller.add(counter);
      if (counter % 5 == 0) {
        controller.close();
        counter++;
        return;
      }
    }
  });
  int i = 1;
  stream.listen((v) {
    print(v);
    Expect.equals(v, i++);
  }, onDone: () {
    Expect.equals(6, i);
    asyncEnd();
  });
  int j = 6;
  stream.listen((v) {
  print(v);
    Expect.equals(v, j++);
  }, onDone: () {
    Expect.equals(11, j);
    asyncEnd();
  });
}

The test works for me but it's unclear for me why. The output is

unittest-suite-wait-for-done
1
6
2
7
3
8
4
9
5
10
unittest-suite-success

Seems that the controller syncronously sends data to the first listener and then to the second. Is this expected? If yes, what's the difference between add and addSync?

@sgrekhov sgrekhov requested a review from lrhn October 15, 2024 15:50
Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a couple of comments.

Copy link
Contributor Author

@sgrekhov sgrekhov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. PTAL.

@sgrekhov sgrekhov requested a review from eernstg October 16, 2024 11:52
@eernstg
Copy link
Member

eernstg commented Oct 16, 2024

help me to understand if the following test is correct or not?

That question would probably fit just fine as an issue on the SDK repository with the label type-question.

@eernstg eernstg merged commit 1a9f057 into dart-lang:master Oct 16, 2024
2 checks passed
@lrhn
Copy link
Member

lrhn commented Oct 16, 2024

BTW could you, please, help me to understand if the following test is correct or not?

It's not wrong. I wouldn't depend on the interleaving always being what it is.

The Stream.multi callbacks here are both synchronous, which means they run immediately to completion when listen is called.
It doesn't matter whether they use add or addSync because no event can be emitted until listen has returned, so all five events are enqueued and a microtask is scheduled to deliver the the next enqueued value.
Both do this, the first one enqueues 1..5+done events and the second 6..10+done, and two microtasks are scheduled.
Then those trigger, each emitting one event and scheduling another microtasks to emit the next event.

It's guaranteed that the streams will emit those events, because all events are computed synchronously.

The scheduling gives the interleaving you see. I would not guarantee any specific interleaving of separate event sources. We can guarantee that the next enqueued event will be delivered in a later microtask, but not precisely which later microtask.

So, working as intended.

copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Oct 18, 2024
2024-10-18 sgrekhov22@gmail.com Fixes dart-lang/co19#2938. Remove string literals identity check (dart-lang/co19#2939)
2024-10-17 sgrekhov22@gmail.com dart-lang/co19#2933. Add MultiStreamController tests. Part 2. (dart-lang/co19#2937)
2024-10-17 sgrekhov22@gmail.com dart-lang/co19#2930. Add MultiStreamController tests. Part 1. (dart-lang/co19#2936)
2024-10-16 sgrekhov22@gmail.com dart-lang/co19#2933. Add tests for `Stream.multi` constructor. (dart-lang/co19#2935)
2024-10-15 sgrekhov22@gmail.com dart-lang/co19#2933. Add tests for `Stream.error` and `Stream.value` constructors. (dart-lang/co19#2934)
2024-10-14 sgrekhov22@gmail.com Fixes dart-lang/co19#2930. Fix failing Stream tests. (dart-lang/co19#2932)

Cq-Include-Trybots: luci.dart.try:analyzer-linux-release-try
Change-Id: Ib7d939f6edaf72a344e75c473e564923d3c811cb
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/390702
Reviewed-by: Erik Ernst <eernst@google.com>
Commit-Queue: Alexander Thomas <athom@google.com>
Reviewed-by: Alexander Thomas <athom@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants