-
Notifications
You must be signed in to change notification settings - Fork 86
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
Avoid StreamController.broadcast #1375
Conversation
4235257
to
0d312c8
Compare
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.
These could be an argument to never return a broadcast stream from any API. To me it is more of a user limitation and single listener streams should be exposed only when explicitly required by the API.
Since this was exposed to the user as a Stream
we used a broadcast stream to allow multiple listeners by default. This also covers single listener case and does not require addition call to asBoradcast
stream.
Also isn't this a breaking change.
Happy to discuss this on the round table. Perhaps we could decide to change it.
1e02db1
to
33fd652
Compare
Pull Request Test Coverage Report for Build 5875858867
💛 - Coveralls |
5de22ad
to
ebccc1b
Compare
There are a number of reasons to prefer not to use broadcast stream here.
asBroadcast
, but it is impossible to go the other way.StreamController
). Each call togetProgressStream
will create a new one anyway, so unless the user explicitly shares the returned stream there is no sharing. Fx. our own tests pass just fine, when converting to a normal stream.SessionProgressNotificationController
owning theStreamController
(and hence theStream
) is passed to native and become a GC root, preventing the GC to collect until realm-core releases it. Unlike single-subscriber streams which are constructed for one-time-use, a broadcast stream is useful to pass around a be subscribed multiple times. If a single cancel call is missed, we have leak.await for
loops. I believe it is important that people opt-in to this.