From 25af27734fbc61188cfe8f5292cb69f3d0fa46b1 Mon Sep 17 00:00:00 2001 From: Florian Huonder Date: Mon, 7 Jan 2019 18:49:27 +0100 Subject: [PATCH] Clearing pendingImages when the cache is cleared or evicted. (#23860) --- .../flutter/lib/src/painting/image_cache.dart | 40 ++++++++--- .../test/painting/image_cache_test.dart | 68 ++++++++++++++++++- .../test/painting/mocks_for_image_cache.dart | 11 +++ 3 files changed, 108 insertions(+), 11 deletions(-) diff --git a/packages/flutter/lib/src/painting/image_cache.dart b/packages/flutter/lib/src/painting/image_cache.dart index 53442d8877..8beb77b7bd 100644 --- a/packages/flutter/lib/src/painting/image_cache.dart +++ b/packages/flutter/lib/src/painting/image_cache.dart @@ -26,7 +26,7 @@ const int _kDefaultSizeBytes = 100 << 20; // 100 MiB /// Generally this class is not used directly. The [ImageProvider] class and its /// subclasses automatically handle the caching of images. class ImageCache { - final Map _pendingImages = {}; + final Map _pendingImages = {}; final Map _cache = {}; /// Maximum number of entries to store in the cache. @@ -48,8 +48,7 @@ class ImageCache { return; _maximumSize = value; if (maximumSize == 0) { - _cache.clear(); - _currentSizeBytes = 0; + clear(); } else { _checkCacheSize(); } @@ -78,8 +77,7 @@ class ImageCache { return; _maximumSizeBytes = value; if (_maximumSizeBytes == 0) { - _cache.clear(); - _currentSizeBytes = 0; + clear(); } else { _checkCacheSize(); } @@ -98,10 +96,15 @@ class ImageCache { /// cache, and when they complete they will be inserted as normal. void clear() { _cache.clear(); + _pendingImages.clear(); _currentSizeBytes = 0; } /// Evicts a single entry from the cache, returning true if successful. + /// Pending images waiting for completion are removed as well, returning true if successful. + /// + /// When a pending image is removed the listener on it is removed as well to prevent + /// it from adding itself to the cache if it eventually completes. /// /// The [key] must be equal to an object used to cache an image in /// [ImageCache.putIfAbsent]. @@ -113,6 +116,11 @@ class ImageCache { /// /// * [ImageProvider], for providing images to the [Image] widget. bool evict(Object key) { + final _PendingImage pendingImage = _pendingImages.remove(key); + if (pendingImage != null) { + pendingImage.removeListener(); + return true; + } final _CachedImage image = _cache.remove(key); if (image != null) { _currentSizeBytes -= image.sizeBytes; @@ -134,7 +142,7 @@ class ImageCache { ImageStreamCompleter putIfAbsent(Object key, ImageStreamCompleter loader(), { ImageErrorListener onError }) { assert(key != null); assert(loader != null); - ImageStreamCompleter result = _pendingImages[key]; + ImageStreamCompleter result = _pendingImages[key]?.completer; // Nothing needs to be done because the image hasn't loaded yet. if (result != null) return result; @@ -166,13 +174,16 @@ class ImageCache { _maximumSizeBytes = imageSize + 1000; } _currentSizeBytes += imageSize; - _pendingImages.remove(key); + final _PendingImage pendingImage = _pendingImages.remove(key); + if (pendingImage != null) { + pendingImage.removeListener(); + } + _cache[key] = image; - result.removeListener(listener); _checkCacheSize(); } if (maximumSize > 0 && maximumSizeBytes > 0) { - _pendingImages[key] = result; + _pendingImages[key] = _PendingImage(result, listener); result.addListener(listener); } return result; @@ -199,3 +210,14 @@ class _CachedImage { final ImageStreamCompleter completer; final int sizeBytes; } + +class _PendingImage { + _PendingImage(this.completer, this.listener); + + final ImageStreamCompleter completer; + final ImageListener listener; + + void removeListener() { + completer.removeListener(listener); + } +} diff --git a/packages/flutter/test/painting/image_cache_test.dart b/packages/flutter/test/painting/image_cache_test.dart index cd554edb90..83da3dced2 100644 --- a/packages/flutter/test/painting/image_cache_test.dart +++ b/packages/flutter/test/painting/image_cache_test.dart @@ -86,7 +86,6 @@ void main() { expect(p.value, equals(16)); // cache has three entries: 3(14), 4(15), 1(16) - }); test('clear removes all images and resets cache size', () async { @@ -139,7 +138,72 @@ void main() { expect(result, null); expect(caughtError, true); }); + + test('already pending image is returned when it is put into the cache again', () async { + const TestImage testImage = TestImage(width: 8, height: 8); + + final TestImageStreamCompleter completer1 = TestImageStreamCompleter(); + final TestImageStreamCompleter completer2 = TestImageStreamCompleter(); + + final TestImageStreamCompleter resultingCompleter1 = imageCache.putIfAbsent(testImage, () { + return completer1; + }); + final TestImageStreamCompleter resultingCompleter2 = imageCache.putIfAbsent(testImage, () { + return completer2; + }); + + expect(resultingCompleter1, completer1); + expect(resultingCompleter2, completer1); + }); + + test('pending image is removed when cache is cleared', () async { + const TestImage testImage = TestImage(width: 8, height: 8); + + final TestImageStreamCompleter completer1 = TestImageStreamCompleter(); + final TestImageStreamCompleter completer2 = TestImageStreamCompleter(); + + final TestImageStreamCompleter resultingCompleter1 = imageCache.putIfAbsent(testImage, () { + return completer1; + }); + + imageCache.clear(); + + final TestImageStreamCompleter resultingCompleter2 = imageCache.putIfAbsent(testImage, () { + return completer2; + }); + + expect(resultingCompleter1, completer1); + expect(resultingCompleter2, completer2); + }); + + test('pending image is removed when image is evicted', () async { + const TestImage testImage = TestImage(width: 8, height: 8); + + final TestImageStreamCompleter completer1 = TestImageStreamCompleter(); + final TestImageStreamCompleter completer2 = TestImageStreamCompleter(); + + final TestImageStreamCompleter resultingCompleter1 = imageCache.putIfAbsent(testImage, () { + return completer1; + }); + + imageCache.evict(testImage); + + final TestImageStreamCompleter resultingCompleter2 = imageCache.putIfAbsent(testImage, () { + return completer2; + }); + + expect(resultingCompleter1, completer1); + expect(resultingCompleter2, completer2); + }); + + test('failed image can successfully be removed from the cache\'s pending images', () async { + const TestImage testImage = TestImage(width: 8, height: 8); + + const FailingTestImageProvider(1, 1, image: testImage).resolve(ImageConfiguration.empty).addListener((ImageInfo image, bool synchronousCall){}, onError: (dynamic exception, StackTrace stackTrace) { + final bool evicationResult = imageCache.evict(1); + expect(evicationResult, isTrue); + }); + }); }); } - diff --git a/packages/flutter/test/painting/mocks_for_image_cache.dart b/packages/flutter/test/painting/mocks_for_image_cache.dart index 05fa6daa56..0425500084 100644 --- a/packages/flutter/test/painting/mocks_for_image_cache.dart +++ b/packages/flutter/test/painting/mocks_for_image_cache.dart @@ -47,6 +47,15 @@ class TestImageProvider extends ImageProvider { String toString() => '$runtimeType($key, $imageValue)'; } +class FailingTestImageProvider extends TestImageProvider { + const FailingTestImageProvider(int key, int imageValue, { ui.Image image }) : super(key, imageValue, image: image); + + @override + ImageStreamCompleter load(int key) { + return OneFrameImageStreamCompleter(Future.sync(() => Future.error('loading failed!'))); + } +} + Future extractOneFrame(ImageStream stream) { final Completer completer = Completer(); void listener(ImageInfo image, bool synchronousCall) { @@ -84,3 +93,5 @@ class ErrorImageProvider extends ImageProvider { return SynchronousFuture(this); } } + +class TestImageStreamCompleter extends ImageStreamCompleter {}