sdk icon indicating copy to clipboard operation
sdk copied to clipboard

Please update `StreamController` documentation

Open sgrekhov opened this issue 1 year ago • 4 comments

See StreamController.addStream() documentation: https://github.com/dart-lang/sdk/blob/4c3e9160f0ebbb0b49ef56d153cf3bd76d0837fd/sdk/lib/async/stream_controller.dart#L295-L297

May be it makes sense to add that in case of MultiStreamController it's also addSync, addErrorSync and closeSync?

StreamController.close(): https://github.com/dart-lang/sdk/blob/4c3e9160f0ebbb0b49ef56d153cf3bd76d0837fd/sdk/lib/async/stream_controller.dart#L252-L253 What about done event? Calling close() twice doesn't produce any issues so it looks like sending done event to the closed stream is allowed.

https://github.com/dart-lang/sdk/blob/4c3e9160f0ebbb0b49ef56d153cf3bd76d0837fd/sdk/lib/async/stream_controller.dart#L255-L256 Looks like a typo. Stream listeners are done sending events? May be listening? And there should be a dot at the end of line 255, not a comma.

https://github.com/dart-lang/sdk/blob/4c3e9160f0ebbb0b49ef56d153cf3bd76d0837fd/sdk/lib/async/stream_controller.dart#L259-L260 It's hard to understand what is meant. Please, rewrite.

Edit: Also the folowing. https://github.com/dart-lang/sdk/blob/4c3e9160f0ebbb0b49ef56d153cf3bd76d0837fd/sdk/lib/async/stream_controller.dart#L267-L268 Seems that "non-broadcast" here should be removed. It's possible to create a broadcast multi-listener stream by Stream.multi but the above will be true for it.

cc @lrhn

sgrekhov avatar Oct 17 '24 09:10 sgrekhov

StreamController.done: https://github.com/dart-lang/sdk/blob/9046485cda4c7ab7b8e934380d80c2e78cfcb4ea/sdk/lib/async/stream_controller.dart#L275-L276

In case of MultiStreamController it's not a single-subscription stream.

https://github.com/dart-lang/sdk/blob/9046485cda4c7ab7b8e934380d80c2e78cfcb4ea/sdk/lib/async/stream_controller.dart#L286-L287

Again, "non-broadcast" seems excessive.

sgrekhov avatar Oct 18 '24 08:10 sgrekhov

StreamController.isPaused: https://github.com/dart-lang/sdk/blob/9046485cda4c7ab7b8e934380d80c2e78cfcb4ea/sdk/lib/async/stream_controller.dart#L222-L224

It's not true for Stream.multi(..., isBroadcast: true).

sgrekhov avatar Oct 18 '24 09:10 sgrekhov

StreamController.onCancel: https://github.com/dart-lang/sdk/blob/9046485cda4c7ab7b8e934380d80c2e78cfcb4ea/sdk/lib/async/stream_controller.dart#L197-L200

This callback is also called on "done" event. Please add it to the documentation.

sgrekhov avatar Oct 18 '24 10:10 sgrekhov

It's not true for Stream.multi(..., isBroadcast: true).

It is! The text is not descriptive, it's prescriptive. That's what a valid broadcast stream must do. The creator of a Stream.multi with the isBroadcast set to true must behave like a broadcast stream, otherwise they're breaking the contract. (Which they can obviously do if they document that, but they should probably just not set isBroadcast to true then.)

This callback is also called on "done" event. Please add it to the documentation.

I'd have thought that was a bug. But ... having checked, it's been like that for 12 years, so let's say it's how it should be. It does ensure that the life-cycle of the controller always goes from onListen to onCancel. (The close method calls _sendDone which callas an internal _cancel() to clear the internal state, which also calls _onCancel().)

lrhn avatar Oct 18 '24 12:10 lrhn