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

EventFlux instance should have dispose() or close() method #12

Closed
jangruenwaldt opened this issue Mar 7, 2024 · 5 comments
Closed

EventFlux instance should have dispose() or close() method #12

jangruenwaldt opened this issue Mar 7, 2024 · 5 comments
Assignees

Comments

@jangruenwaldt
Copy link
Contributor

For example, in the following line is a StreamSubscription that is never cancelled, this could cause memory leaks:

      ///Applying transforms and listening to it
      data.stream
          .transform(const Utf8Decoder())
          .transform(const LineSplitter())
          .listen(...)

Things like this and the other state should be cleaned up when dispose() is called.

@Imgkl Imgkl self-assigned this Mar 10, 2024
@Imgkl Imgkl added the bug Something isn't working label Mar 10, 2024
@Imgkl
Copy link
Owner

Imgkl commented Mar 30, 2024

Doesn't closing the client, automatically close this data stream as well? Also there's no close/disconnect method for StreamedResponse class

@Imgkl Imgkl added Need more context Need more context and removed bug Something isn't working labels Mar 30, 2024
@jangruenwaldt
Copy link
Contributor Author

When calling .listen(...) on a stream, a StreamSubscription is created:

https://api.flutter.dev/flutter/dart-async/StreamSubscription-class.html

This subscription should be cancelled once it is not needed anymore.

For example, once the connection is lost and we have to reconnect, I think this code is called again, and a new subscription is created. (IIRC - please double check though 😄) But in general, its a good practice to keep track of stream subscriptions, and cancel them on dispose().

@Imgkl Imgkl removed the Need more context Need more context label Apr 19, 2024
@Imgkl
Copy link
Owner

Imgkl commented Apr 19, 2024

@jangruenwaldt Thanks for the resource, I found it useful.

I found what you were mentioning. I've fixed it. Should be available in the next release.

@Imgkl
Copy link
Owner

Imgkl commented Apr 19, 2024

Keeping this issue open still the fix is released.

Imgkl added a commit that referenced this issue Apr 24, 2024
@Imgkl
Copy link
Owner

Imgkl commented Apr 24, 2024

This is fixed on the latest version of EventFlux, v1.7.0.

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

No branches or pull requests

2 participants