[map] Remove empty blobs, ignore acquisition exceptions during eviction

The WMS server can return empty 200 responses under load. Empty blobs were being persisted in the cache (and not refreshed), making tiles received this way permanently undisplayable.

There may be an opportunity to evict any blobs that fail to decode, but the logic for this would be more complex as decoding can also flake when under load.

Furthermore, when the map is moved after long-running tile fetches that eventually fail, exceptions could be dropped in one of at least two ways:
 * via the image provider (`OneFrameImageStreamCompleter`), if the map tile layer has unsubscribed from the image stream, as when the tile scrolls offscreen before the image loading fails, or
 * via the async cache, if enough tiles are scrolled to cause eviction. The `onEvict` for images has to wait for them to load to dispose them. (There's an opportunity for improvement there.)

The `OneFrameImageStreamCompleter`/`FlutterError.reportError` behavior is acceptable and will be silent in release mode, but note that it can fail tests.
The onEvict behavior should ignore these exceptions as they would be handled elsewhere if they matter.

Fixes #83

app/lib/persistence/blob_cache.dart

@@ -104,6 +104,11 @@ class BlobCache {
     }
   }
 
+  void remove(String key) {
+    blobs.delete(key);
+    metadata.delete(key);
+  }
+
   void evict(EvictionPolicy shouldEvict) {
     // for logging
     int evictions = 0;

app/lib/providers/wms_tile_provider.dart

@@ -103,7 +103,16 @@ class TileKey extends TileLocator {
   String toString() => '$type@$zoom:(${coordinate.x},${coordinate.y})+$lod';
 }
 
-Future<Image> _decode(Uint8List data) async {
+class TileException implements Exception {
+  TileException(this.message, this.key);
+  final String message;
+  final TileKey key;
+
+  @override
+  String toString() => 'TileException: $message\nTileKey: $key';
+}
+
+Future<Image> decodeImage(Uint8List data) async {
   final buffer = await ImmutableBuffer.fromUint8List(data);
   final ImageDescriptor imageDescriptor;
   try {
@@ -164,6 +173,7 @@ class WmsTileProvider extends TileProvider {
     this.maxOversample = 2,
     this.fetchLod = 0,
     required this.params,
+    this.imageDecoder = decodeImage,
   });
 
   http.Client client;
@@ -185,6 +195,8 @@ class WmsTileProvider extends TileProvider {
 
   WmsParams params;
 
+  Future<Image> Function(Uint8List) imageDecoder;
+
   @override
   void dispose() {
     cache.close();
@@ -215,25 +227,51 @@ class WmsTileProvider extends TileProvider {
     if (response.statusCode == HttpStatus.ok) {
       return response.bodyBytes;
     } else {
-      throw response;
+      throw TileException(
+        '$url returned status code ${response.statusCode}',
+        TileKey.forLocator(locator, tileType),
+      );
     }
   }
 
   final _memoryCache = AsyncCache<TileLocator, Image>.persistent(
     MemoryCache(
       capacity: 32,
-      onEvict: (image) async => (await image).dispose(),
+      onEvict: (value) async {
+        final Image image;
+        try {
+          image = await value;
+        } on Object {
+          // Errors here are handled elsewhere if they matter, so ignore
+          // silently during eviction.
+          return;
+        }
+        image.dispose();
+      },
     ),
   );
 
-  Future<Uint8List> getImageData(TileLocator locator) async =>
-      cache[TileKey.forLocator(locator, tileType).toString()] ??=
-          await fetchImageData(locator);
+  Future<Uint8List> getImageData(TileLocator locator) async {
+    final key = TileKey.forLocator(locator, tileType);
+    final $key = key.toString();
+    final imageData = cache[$key] ??= await fetchImageData(locator);
+
+    if (imageData.isEmpty) {
+      // We could throw during fetchImageData, but there's also the possibility
+      // of bad data already in the cache from older versions, which we'll want
+      // to clean up.
+      cache.remove($key);
+      // This will fail decoding anyway, but let's short circuit and throw now.
+      throw TileException('Empty tile data.', key);
+    } else {
+      return imageData;
+    }
+  }
 
   Future<Image> getComponentImage(TileLocator locator) async =>
       (await (_memoryCache.computeIfAbsent(locator, () async {
         final data = await getImageData(locator);
-        return _decode(data);
+        return imageDecoder(data);
       })))
           .clone();
 

app/test/map_test.dart

@@ -1,9 +1,13 @@
+import 'dart:async';
+import 'dart:io';
 import 'dart:math';
 
 import 'package:flutter/material.dart';
 import 'package:flutter_map/flutter_map.dart' hide MapController;
 import 'package:flutter_test/flutter_test.dart';
+import 'package:http/http.dart' as http;
 import 'package:latlng/latlng.dart';
+import 'package:mockito/mockito.dart';
 import 'package:motion_sensors/motion_sensors.dart';
 import 'package:trip_planner_aquamarine/providers/ofs_client.dart';
 import 'package:trip_planner_aquamarine/providers/trip_planner_client.dart';
@@ -137,5 +141,68 @@ void main() {
         ),
       );
     });
+
+    testWidgets('handles tile retrieval errors', (tester) async {
+      when(harness.client.get(any))
+          .thenAnswer((_) async => http.Response('', 404));
+
+      await tester.pumpWidget(MapHarness(harness: harness));
+      await tester.pumpAndSettle();
+    });
+
+    testWidgets('handles empty tiles', (tester) async {
+      when(harness.client.get(any)).thenAnswer(
+          (_) async => http.Response.bytes(const [], HttpStatus.ok));
+
+      await tester.pumpWidget(MapHarness(harness: harness));
+      await tester.pumpAndSettle();
+    });
+
+    testWidgets('handles tile decoding errors', (tester) async {
+      when(harness.client.get(any)).thenAnswer(
+          (_) async => http.Response.bytes(const [0], HttpStatus.ok));
+
+      await tester.pumpWidget(MapHarness(harness: harness));
+      await tester.pumpAndSettle();
+    });
+
+    // AsturaPhoenix/trip_planner_aquamarine#83
+    // When the map is moved after long-running tile fetches that eventually
+    // fail, exceptions could be dropped in one of at least two ways:
+    // * via the image provider (OneFrameImageStreamCompleter), if the map tile
+    //   layer has unsubscribed from the image stream, as when the tile scrolls
+    //   offscreen before the image loading fails, or
+    // * via the async cache, if enough tiles are scrolled to cause eviction.
+    //   The onEvict for images has to wait for them to load to dispose them.
+    //   (There's an opportunity for improvement there.)
+    //
+    // The OneFrameImageStreamCompleter/FlutterError.reportError behavior is
+    // acceptable and will be silent in release mode, but note that it can fail
+    // tests.
+    testWidgets('handles tile exceptions after map movement', (tester) async {
+      final response = Completer<http.Response>();
+      when(harness.client.get(any)).thenAnswer((_) => response.future);
+
+      await tester.pumpWidget(MapHarness(harness: harness));
+
+      // Scroll enough to cause both tile unsubscription and cache eviction.
+      await tester.fling(find.byType(FlutterMap), const Offset(0, 1600), 1000);
+      await tester.pumpAndSettle();
+
+      // The OneFrameImageStreamCompleter behavior of using
+      // FlutterError.reportError with silent = true is an acceptable way of
+      // handling image errors that are dropped due to tiles scrolling
+      // offscreen. To acknowledge this in the test, we need to do the
+      // equivalent of tester.takeException on all such exceptions. We can do
+      // something like this by overriding the error handler.
+      addTearDown(() => FlutterError.onError = FlutterError.presentError);
+      FlutterError.onError = (details) {
+        if (!details.silent) {
+          FlutterError.presentError(details);
+        }
+      };
+
+      response.complete(http.Response.bytes(const [], HttpStatus.ok));
+    });
   });
 }

app/test/util/cache.dart

@@ -39,5 +39,7 @@ class FakeBlobCache extends Fake implements BlobCache {
   @override
   void operator []=(String key, Uint8List value) => data[key] = value;
   @override
+  void remove(String key) => data.remove(key);
+  @override
   void close() {}
 }

app/test/wms_tile_provider_test.dart

@@ -7,7 +7,6 @@ import 'package:mockito/annotations.dart';
 import 'package:mockito/mockito.dart';
 import 'package:test/test.dart';
 import 'package:test_data/empty.png.dart';
-import 'package:trip_planner_aquamarine/platform/compositor.dart';
 import 'package:trip_planner_aquamarine/providers/wms_tile_provider.dart';
 
 import 'util/cache.dart';
@@ -71,21 +70,37 @@ void main() {
       (_) async => http.Response.bytes(kEmptyPng, HttpStatus.ok),
     );
 
-    final actualDecode = CompositorImage.decode;
-    CompositorImage.decode = (_) => Future.error('Mock flake');
+    testTileProvider.imageDecoder = (_) => Future.error('Mock flake');
 
     const locator = TileLocator(0, Point(0, 0), 0);
-    try {
-      await testTileProvider.getTileContent(locator);
-    } on String {
-      // expected
-    }
+    await expectLater(
+        () => testTileProvider.getTileContent(locator), throwsA(isA<String>()));
     verify(mockHttpClient.get(any));
 
-    CompositorImage.decode = actualDecode;
+    testTileProvider.imageDecoder = decodeImage;
 
     await testTileProvider.getTileContent(locator);
     // The http call should still be cached.
     verifyNoMoreInteractions(mockHttpClient);
   });
+
+  // The server can sometimes return empty 200 responses, which we don't want to
+  // cache.
+  test('does not cache empty data', () async {
+    when(mockHttpClient.get(any)).thenAnswer(
+      (_) async => http.Response.bytes(const [], HttpStatus.ok),
+    );
+
+    const locator = TileLocator(0, Point(0, 0), 0);
+    await expectLater(() => testTileProvider.getTileContent(locator),
+        throwsA(isA<Exception>()));
+    verify(mockHttpClient.get(any));
+
+    when(mockHttpClient.get(any)).thenAnswer(
+      (_) async => http.Response.bytes(kEmptyPng, HttpStatus.ok),
+    );
+
+    await testTileProvider.getTileContent(locator);
+    verify(mockHttpClient.get(any));
+  });
 }