Skip to content
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

StreamController.close returns a Future that never completes #19095

Closed
nex3 opened this issue May 30, 2014 · 6 comments
Closed

StreamController.close returns a Future that never completes #19095

nex3 opened this issue May 30, 2014 · 6 comments
Assignees
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-async type-documentation A request to add or improve documentation

Comments

@nex3
Copy link
Member

nex3 commented May 30, 2014

Consider the following code:

    void main() {
      var controller = new StreamController();
      controller.close().then((_) => print("done closing"));
    }

This exits without printing anything. The [controller.close] Future is never completing.

With issues as straightforward as this and issue #18586 going undetected by the library authors, I'm worried that the test coverage for dart:async is woefully insufficient.

@lrhn
Copy link
Member

lrhn commented Jun 2, 2014

The future returned by StreamController.close completes when the done event has been delivered.
In this case, there is no listen on the non-broadcast stream, so the done event is never sent, and the future never completes.
If you add a c.stream.listen(null);, the returned future will complete.

I admit the documentation on this future is lacking. The close method that returns a future is inherited from StreamConsumer, but the documentation isn't clear what it means for the close call to be complete. For a controller, it means that the done event has been sent. I'll update the documentation to make this clear.


Added AsDesigned label.

@nex3
Copy link
Member Author

nex3 commented Jun 2, 2014

This is extremely confusing behavior, and contrary to the behavior of other [close] methods. Methods like [Socket.close] close when the underlying resources have been released, and methods like [StreamSubscription.cancel] return "null" if there's no additional work to wait on (which is worse than returning a completed Future, but still better than this).

Not only is returning a Future that you know will never complete confusing, it's extremely likely to cause deadlocks in unsuspecting programs. Please either follow [StreamSubscription.cancel]'s behavior or return a completed future if there are no events to deliver.


Added Triaged label.

@lrhn
Copy link
Member

lrhn commented Jun 3, 2014

There is an event to deliver: The done event.
And the future will complete as soon as someone starts listening on the stream.

Having a non-broadcast controller and stream that never gets a listener is not recommended. If you add errors to the stream, they are also never reported anywhere.
If you wait for the done event to be delivered, it won't be. There is nothing magic about that.

The returned future waits for the done event to be delivered, whether there is a listener or not. At that point it assumes that the listener has cleaned up and completed what it needs to complete.
There is no difference between a controller that has delivered all its events and then you call 'close', and one that you call 'close' on as the first thing, and it shouldn't change behavior depending on when you listen.

The 'close' method with a future return is inherited from StreamSink. That was added as a request from dart:io, because they needed to know when a StreamSink was done processing its data - when the done event had been delivered.
I admit that the future returned by close can mostly be ignored on a StreamController (and it was a mistake to make StreamController be a StreamSink, we should have had a wrapper instead of making all StreamControllers more complex), but when it is used, it should act the way it currently does.


Added AsDesigned label.

@nex3 nex3 added Type-Defect area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-async closed-as-intended Closed as the reported issue is expected behavior labels Jun 3, 2014
@natebosch
Copy link
Member

Reopening since this the documentation is not updated and this is still confusing. The future will also never complete if the subscription was paused before the done even was delivered - this can easily happen in tests that use StreamQueue.

I think it's worth updating the documentation to make it clear that it's generally safe to ignore the returned Future, especially since usage of the unawaited_future lint is becoming more common.

@LewisHolliday
Copy link

I think the docs or the implementation is incorrect for broadcast stream controllers.

The docs say:
'The returned future is the same future provided by done. It is completed when the stream listeners is done sending events, This happens either when the done event has been sent, or when the subscriber on a single-subscription stream is canceled.'

A broadcast stream controller will send the done event even if listeners are paused, so some broadcast events may not have been received yet when the returned future completes.

import 'dart:async';

void main() async {
  final c = StreamController<int>.broadcast();
  final stream = c.stream;

  final sub = stream.listen(print);

  c.add(100);
 
  sub.pause();

  await c.close(); //nothing completes this
  print('closed'); 
}

@natebosch natebosch reopened this May 9, 2023
@lrhn
Copy link
Member

lrhn commented May 9, 2023

That is misleading. The done future is completed when the done event has been processed, not just sent.
A paused listener will not process the event.

I think it's slightly weird that a broadcast controller cares about its listeners being there or not, but there is a practical reason for it.

The reason for the "done future" existing at all, is that the stream controller/owner should not start cleaning up resources required by events until all events have been delivered. Even a broadcast stream can need that.
When the done future completes, it's guaranteed that there are no pending data (or error) events which have not been delivered, and which will be delivered later.

Stream listeners can pause their subscription until they have fully processed an event, that way they can tell the stream source that they're not done yet. When they receive the done event, they report that back, and both sides now know that all events have been fully processed.

So there is a reason. It's just not documented very well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-async type-documentation A request to add or improve documentation
Projects
None yet
Development

No branches or pull requests

5 participants