Fix #785 Cancel chunk events stream subscription when imageStream is disposed

[davidmartos96]

✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)

Bug fix

⤵️ What is the current behavior?

#785 There seems to be a situation where a new chunkEvent is being reported and the Image Stream has already been disposed.

🆕 What is the new behavior (if this is a feature change)?

It makes sure to remove the chunk events Stream subscription once the ImageStream is disposed. Since the completer base class does not expose the properties that indicate when it has been disposed, we need to replicate some of the logic.

This adds a listener for when the last ImageStream is removed. This is when the ImageStream is disposed, so we have a chance to close the chunkEvents StreamController.
I wasn't able to reproduce the original issue on my device, but I'm seeing thousands of "silent" error reports from the checkDisposed assert in user stacktraces.
Fortunately, the test case added provides a way to reproduce the issue.

💥 Does this PR introduce a breaking change?

No

🐛 Recommendations for testing

A test case has been included

📝 Links to relevant issues/docs

#785

🤔 Checklist before submitting

• All projects build
• Follows style guide lines (code style guide)
• Relevant documentation was updated
• Rebased onto current develop

[davidmartos96]

I've included a test inspired by this one from the Flutter repo
https://github.com/flutter/flutter/blob/35daf375543f377921ffe9e33e27392a27363f97/packages/flutter/test/painting/image_stream_test.dart#L815

After adding the test I noticed that the first commit had flaws, so I've taken a different route for the implementation, basing of the ImageStreamCompleter code from flutter.

Unfortunately, since the ImageStreamCompleter base class has _disposed as a private property and is not decorated with @protected, there is some logic that the base class does that needs to be replicated. I used double underscores for those properties from the super class.

Here are the important lines from the Flutter repo

_maybeDispose
https://github.com/flutter/flutter/blob/35daf375543f377921ffe9e33e27392a27363f97/packages/flutter/lib/src/painting/image_stream.dart#L595

Stream subscription cancel
https://github.com/flutter/flutter/blob/35daf375543f377921ffe9e33e27392a27363f97/packages/flutter/lib/src/painting/image_stream.dart#L1020

keepAlive handle
https://github.com/flutter/flutter/blob/35daf375543f377921ffe9e33e27392a27363f97/packages/flutter/lib/src/painting/image_stream.dart#L443

[renefloor]

@davidmartos96 I think it looks quite good in general. Will also have a closer look at the links you share. Is there a reason to use 2 underscores for the variables and methods instead of just 1?

[codecov]

Codecov Report

Base: 82.81% // Head: 84.72% // Increases project coverage by +1.90% 🎉

Coverage data is based on head (ac60e35) compared to base (3fcd65a).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #792      +/-   ##
===========================================
+ Coverage    82.81%   84.72%   +1.90%     
===========================================
  Files            4        4              
  Lines          192      216      +24     
===========================================
+ Hits           159      183      +24     
  Misses          33       33              

Impacted Files	Coverage Δ
...c/image_provider/multi_image_stream_completer.dart	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[davidmartos96]

@renefloor

Is there a reason to use 2 underscores for the variables and methods instead of just 1?

There are 2 reasons. One is that the IDE and Dart analyzer was getting confused that they were the same name as the private properties from the super class. The second is that it is some way to distinguish them from the other main logic.

Ideally a PR should be opened in the Flutter repo so that is easier to subclass the completer class in third party packages.

[vlazdra]

Hey @renefloor sorry to bother you, but do you have any plans on merging/releasing this any time soon? I have 40k event reports logged to Sentry because of this issue, and it's eating up my plan bit by bit. 😄

[davidmartos96]

@vlazdra This was my situation as well with Sentry. While this is not merged you can use the following in your pubspec

dependency_overrides:
  cached_network_image:
    git:
      url: https://github.com/davidmartos96/flutter_cached_network_image
      path: cached_network_image
      ref: ce6d67111c727c3a44849d67d1c730985d999c9e

[vlazdra]

@vlazdra This was my situation as well with Sentry. While this is not merged you can use the following in your pubspec

dependency_overrides:
  cached_network_image:
    git:
      url: https://github.com/davidmartos96/flutter_cached_network_image
      path: cached_network_image
      ref: ce6d67111c727c3a44849d67d1c730985d999c9e

Hey! Thanks for the reply, and the details as well. 🙌

I have nothing against implementing the custom ref commit in the test version of the app. I was however asking because I don't fell comfortable going into production with it. I do understand that nothing major could occur based on the source code, still would be great to have a release.

Hope you understand where I'm coming from. Not trying to hurry anything, or be ungrateful. Just trying to be safe. 🙂

[vlazdra]

Hey @renefloor, it's been a month since this PR was created. Have you had a chance to check it out?
Also, do you have a timeline when this could be released?

Thank you! 😄

[renefloor]

Sorry for the delay. Going to release soon.

[davidmartos96]

Sorry for the delay. Going to release soon.

Great to hear that!
@renefloor I got notified of the CI not working. I believe it's because the subosito/flutter-action@v1 action. From other issues I read it looks like v2 fixed that issue.
subosito/flutter-action#146
subosito/flutter-action#161

[renefloor]

Thanks for the references. The failures had nothing to do with the changes in the PR, so I didn't want to spend time on it for this issue and was going to have a look at it this weekend, but now I can already quickly try v2 :)