flutter / flutter

Flutter makes it easy and fast to build beautiful apps for mobile and beyond
https://flutter.dev
BSD 3-Clause "New" or "Revised" License
165.65k stars 27.35k forks source link

`State.dispose` is synchronous but `StreamSubscription.cancel` may be asynchronous #125849

Open AsturaPhoenix opened 1 year ago

AsturaPhoenix commented 1 year ago

Is there an existing issue for this?

Use case

I have a StatefulWidget that sets up a stream in its initState. onListen, the stream starts a Timer. In dispose, all StreamSubscriptions are canceled, which also cancels all Timers. However, StreamSubscription.cancel may be asynchronous (and it seems to be so for streams that include an addStream?), which can result in timers leaking beyond dispose.

Demo: https://dartpad.dev/?id=aa9938512f792217570748e5123a6e7d

A widget smoke test exposes this. It is unclear to me whether there is a more serious case where events may be delivered to a subscriber after dispose even though cancel was called. If this case exists, StreamBuilder may be affected as well, as it similarly has to call cancel unawaited in its dispose.

This use case was discussed briefly on #24166, but the resolution is not explicitly applicable since simply waiting for the timer to complete is a different test case from ensuring that timers are properly canceled.

Seeing as how we end up needing a runAsync to test the cancellation case, there may be a bug behind this, possibly an instance of dart-lang/sdk#40131.

Related issue: #91814

Proposal

Naively, I might imagine that dispose might be allowed to be asynchronous, but I also imagine that could be a tough sell. Possibly, we might do with documented guidance on whether an event may indeed be delivered after cancel (so, whether listeners should be gated on mounted), with maybe guidance on what to do about the outstanding timer warning.

The current wording at StreamSubscription.cancel states:

After this call, the subscription no longer receives events.

It is not clear to me whether this applies to after the call has been started but before the future has completed. The author of #91814 would seem to concur.

lrhn commented 12 months ago

It is not clear to me whether this applies to after the call has been started but before the future has completed.

It does (or should, for a correctly implemented steam subscription).

Immediately, synchronously, when calling cancel, the stream subscription is cancelled. It should no longer deliver any events. The returned future is only there in case the canceller wants to know whether all underlying resources have been released, which is sometimes useful, and to provide a way to expose any error happening during clean-up.

If you don't care, you can just call .ignore() on the future and forget about it.

a-siva commented 9 months ago

@lrhn is there anything actionable from the Dart side for this issue, if not can we just close it.

lrhn commented 9 months ago

Not directly. We could consider whether the SteamSubscription.cancel documentation can be improved, and maybe suggest that it's fine to .ignore() cancellation futures if one doesn't need to wait for cleanup.