diff --git a/lib/providers/collab/cloud_media_cache_provider.dart b/lib/providers/collab/cloud_media_cache_provider.dart index f6f0679c..81a0ab54 100644 --- a/lib/providers/collab/cloud_media_cache_provider.dart +++ b/lib/providers/collab/cloud_media_cache_provider.dart @@ -51,16 +51,19 @@ class CloudImageDownloadException implements Exception { /// How long one image download may take before it counts as failed. const cloudImageDownloadTimeout = Duration(seconds: 60); -/// Downloads a cloud image's bytes from [url]. A signed URL the host refuses -/// (401, 403, or 404: it expired) is swapped once for [freshUrl] from the -/// server. Throws [CloudImageDownloadException] when the bytes don't come, -/// and a [TimeoutException] when a request stalls. +/// Downloads a cloud image's bytes from [url], through [client] when given +/// (the caller owns it and closes it). A signed URL the host refuses (401, +/// 403, or 404: it expired) is swapped once for [freshUrl] from the server. +/// Throws [CloudImageDownloadException] when the bytes don't come, and a +/// [TimeoutException] when a request stalls. Future downloadCloudImageBytes( String url, { required Future Function() freshUrl, + http.Client? client, }) async { Future get(String url) => - http.get(Uri.parse(url)).timeout(cloudImageDownloadTimeout); + (client?.get(Uri.parse(url)) ?? http.get(Uri.parse(url))) + .timeout(cloudImageDownloadTimeout); var response = await get(url); if (const {401, 403, 404}.contains(response.statusCode)) { final refreshed = await freshUrl(); diff --git a/lib/screenshot/capture_images.dart b/lib/screenshot/capture_images.dart index 36cc554e..5337fed8 100644 --- a/lib/screenshot/capture_images.dart +++ b/lib/screenshot/capture_images.dart @@ -26,10 +26,13 @@ class CaptureImagesUnavailable implements Exception { : 'CaptureImagesUnavailable: $cause'; } -/// Fetches the bytes behind a cloud image's [url]. +/// Fetches the bytes behind a cloud image's [url] through [client], the +/// capture's own. Call `client.get`, never package:http's top-level `get`: +/// that one closes whatever client it picked up when it finishes. typedef CaptureImageFetcher = Future Function( String imageId, String url, + http.Client client, ); /// What each image in a capture paints, by image id, decoded and held in @@ -69,17 +72,15 @@ Future resolveCaptureImages( throw const CaptureImagesUnavailable.stillLoading(); } // Downloads run together, so a page's wait is its slowest image rather - // than the sum of them. They share one client, closed when this returns, - // so a capture that stops early aborts the downloads it no longer needs; - // a download nobody awaits must not surface as an unhandled error. + // than the sum of them. They share one client, closed only when this + // returns, so a capture that stops early aborts the downloads it no + // longer needs; a download nobody awaits must not surface as an + // unhandled error. final client = http.Client(); final downloads = { for (final MapEntry(key: imageId, value: source) in sources.entries) if (source case RemoteImageUrl(:final url)) - imageId: http.runWithClient( - () => _guard(() => fetch(imageId, url)), - () => client, - )..ignore(), + imageId: _guard(() => fetch(imageId, url, client))..ignore(), }; final resolved = {}; final holds = <_HeldImage>[]; diff --git a/lib/screenshot/page_screenshot.dart b/lib/screenshot/page_screenshot.dart index a0259f1e..8f0d7132 100644 --- a/lib/screenshot/page_screenshot.dart +++ b/lib/screenshot/page_screenshot.dart @@ -53,8 +53,9 @@ Future captureEditorPage(WidgetRef ref) async { (id: image.id, fileExtension: image.fileExtension), ), }, - fetch: (imageId, url) => downloadCloudImageBytes( + fetch: (imageId, url, client) => downloadCloudImageBytes( url, + client: client, freshUrl: () { final linkView = ref.read(shareLinkViewProvider); return ref.read(convexStrategyRepositoryProvider).getImageAssetUrl( diff --git a/test/capture_images_test.dart b/test/capture_images_test.dart index ae519098..8507ee2f 100644 --- a/test/capture_images_test.dart +++ b/test/capture_images_test.dart @@ -4,6 +4,7 @@ import 'dart:typed_data'; import 'package:flutter/painting.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:icarus/providers/collab/cloud_media_cache_provider.dart'; import 'package:icarus/providers/strategy_image_source.dart'; import 'package:icarus/screenshot/capture_images.dart'; import 'package:http/http.dart' as http; @@ -39,6 +40,30 @@ class _SlowClient extends http.BaseClient { } } +/// Like a browser client: once closed it refuses every request. Answers +/// each URL with its status after its delay in milliseconds. +class _RefusingAfterCloseClient extends http.BaseClient { + _RefusingAfterCloseClient(this.answers, this.body); + + final Map answers; + final Uint8List body; + final requested = []; + var closed = false; + + @override + Future send(http.BaseRequest request) async { + if (closed) throw http.ClientException('Client is closed', request.url); + requested.add(request.url.toString()); + final (status, delay) = answers[request.url.toString()]!; + await Future.delayed(Duration(milliseconds: delay)); + if (closed) throw http.ClientException('Request aborted', request.url); + return http.StreamedResponse(Stream.value(body), status); + } + + @override + void close() => closed = true; +} + void main() { TestWidgetsFlutterBinding.ensureInitialized(); @@ -65,7 +90,7 @@ void main() { 'uploading': ImageBytes(pending), 'failed': const ImageFailed(), }, - fetch: (imageId, url) async { + fetch: (imageId, url, _) async { requests.add((imageId, url)); return fetched; }, @@ -87,7 +112,7 @@ void main() { 'remote': const RemoteImageUrl('https://media.example.com/remote.png'), 'file': LocalImageFile(filePath), }, - fetch: (_, __) async => fetched, + fetch: (_, __, ___) async => fetched, ); // The capture's widgets ask for the same keys and find them decoded. @@ -116,7 +141,7 @@ void main() { for (final id in ['a', 'b', 'c', 'd']) id: RemoteImageUrl('https://media.example.com/$id.png'), }, - fetch: (imageId, _) async { + fetch: (imageId, _, __) async { started.add(imageId); await Future.delayed(const Duration(milliseconds: 200)); return _png(imageId.codeUnitAt(0)); @@ -140,9 +165,9 @@ void main() { 'broken': const RemoteImageUrl('https://media.example.com/x.png'), 'slow': const RemoteImageUrl('https://media.example.com/s.png'), }, - fetch: (imageId, url) async { + fetch: (imageId, url, client) async { if (imageId == 'broken') throw Exception('offline'); - return (await http.get(Uri.parse(url))).bodyBytes; + return (await client.get(Uri.parse(url))).bodyBytes; }, ), () => client, @@ -153,6 +178,38 @@ void main() { expect(client.closed, isTrue); }); + test('downloads and URL refreshes share a client nothing closes early', + () async { + // A browser client refuses every request once closed, and aborts the + // ones in flight. One download finishing must not close it for the + // others, or for a refresh after an expired URL. + final client = _RefusingAfterCloseClient({ + 'https://media.example.com/fast.png': (200, 10), + 'https://media.example.com/slow.png': (200, 150), + 'https://media.example.com/expired.png': (403, 20), + 'https://media.example.com/fresh.png': (200, 20), + }, _png(40)); + final images = await http.runWithClient( + () => resolveCaptureImages( + { + for (final id in ['fast', 'slow', 'expired']) + id: RemoteImageUrl('https://media.example.com/$id.png'), + }, + fetch: (imageId, url, client) => downloadCloudImageBytes( + url, + client: client, + freshUrl: () async => 'https://media.example.com/fresh.png', + ), + ), + () => client, + ); + addTearDown(images.release); + + expect(images.sources.values, everyElement(isA())); + expect(client.requested, hasLength(4)); + expect(client.closed, isTrue); + }); + test('an image still loading stops the capture before any download', () async { final started = []; @@ -162,7 +219,7 @@ void main() { 'remote': const RemoteImageUrl('https://media.example.com/r.png'), 'loading': const ImageLoading(), }, - fetch: (imageId, _) async { + fetch: (imageId, _, __) async { started.add(imageId); return fetched; }, @@ -176,7 +233,7 @@ void main() { await expectLater( resolveCaptureImages( {'loading': const ImageLoading()}, - fetch: (_, __) async => fetched, + fetch: (_, __, ___) async => fetched, ), throwsA( isA() @@ -195,7 +252,7 @@ void main() { await expectLater( resolveCaptureImages( {'remote': const RemoteImageUrl('https://media.example.com/r.png')}, - fetch: (_, __) async => throw failure, + fetch: (_, __, ___) async => throw failure, ), throwsA( isA() @@ -213,7 +270,7 @@ void main() { await expectLater( resolveCaptureImages( {'remote': const RemoteImageUrl('https://media.example.com/r.png')}, - fetch: (_, __) async => Uint8List.fromList([1, 2, 3, 4]), + fetch: (_, __, ___) async => Uint8List.fromList([1, 2, 3, 4]), ), throwsA( isA()