From 088055b051a05710ea8e2f68673963bc7f1b7e03 Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Tue, 29 Sep 2026 03:23:23 -0400 Subject: [PATCH] Keep a capture's shared download client open until the capture ends A capture's downloads shared one client through http.runWithClient so a failed capture could abort the rest. But package:http's top-level get closes whatever client it picked up when its request finishes, so the first finished download closed the shared client: in a browser that aborts the other downloads in flight and refuses a refresh after an expired URL, and a screenshot of a page with two or more cloud images failed with "Couldn't load the images on this page." The fetcher now receives the capture's client and downloadCloudImageBytes calls client.get on it, so only the capture closes it, once it is done. A test serves three images (one refreshed after an expired URL) through a client that, like BrowserClient, refuses requests once closed; it fails with the old behaviour. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../collab/cloud_media_cache_provider.dart | 13 ++-- lib/screenshot/capture_images.dart | 17 +++-- lib/screenshot/page_screenshot.dart | 3 +- test/capture_images_test.dart | 75 ++++++++++++++++--- 4 files changed, 85 insertions(+), 23 deletions(-) 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()