diff --git a/mobile/lib/features/channels/message_content/media_carousel.dart b/mobile/lib/features/channels/message_content/media_carousel.dart index 2006e2c520..fb5dc1bda1 100644 --- a/mobile/lib/features/channels/message_content/media_carousel.dart +++ b/mobile/lib/features/channels/message_content/media_carousel.dart @@ -36,16 +36,21 @@ class _MessageGalleryPrecache extends HookWidget { } } +// Loads that leave the window are abandoned, so scrolling past previews on a +// slow link frees both workers instead of finishing downloads nobody needs. class _GalleryPrecache { BuildContext? _context; List> _window = []; final _ready = >{}; - final _loading = >{}; + final _loading = , _PrecacheLoad>{}; void update(BuildContext context, List> window) { _context = context; _window = window; _ready.removeWhere((provider) => !window.contains(provider)); + for (final provider in _loading.keys.toList()) { + if (!window.contains(provider)) _loading.remove(provider)!.cancel(); + } _drain(); } @@ -54,17 +59,18 @@ class _GalleryPrecache { if (context == null || !context.mounted) return; for (final provider in _window) { if (_loading.length >= 2) break; - if (_ready.contains(provider) || _loading.contains(provider)) continue; - _loading.add(provider); - unawaited(_load(context, provider)); + if (_ready.contains(provider) || _loading.containsKey(provider)) continue; + final load = _loading[provider] = _PrecacheLoad( + provider, + createLocalImageConfiguration(context), + () => _finish(provider), + ); + // A cached image settles synchronously, so start once it is tracked. + load.start(); } } - Future _load( - BuildContext context, - ImageProvider provider, - ) async { - await precacheImage(provider, context, onError: (_, _) {}); + void _finish(ImageProvider provider) { _loading.remove(provider); if (_window.contains(provider)) _ready.add(provider); _drain(); @@ -74,6 +80,45 @@ class _GalleryPrecache { _context = null; _window = []; _ready.clear(); + for (final load in _loading.values) { + load.cancel(); + } + _loading.clear(); + } +} + +// Like precacheImage, but cancellable before the first frame. +class _PrecacheLoad { + _PrecacheLoad(this.provider, this.config, this.onDone); + + final ImageProvider provider; + final ImageConfiguration config; + final VoidCallback onDone; + late final _stream = provider.resolve(config); + late final _listener = ImageStreamListener((image, _) { + image.dispose(); + _settle(); + }, onError: (_, _) => _settle()); + bool _settled = false; + + void start() => _stream.addListener(_listener); + + void _settle() { + if (_settled) return; + _settled = true; + // Like precacheImage, keep listening until the end of the frame so a + // widget can subscribe before the image cache may drop the live image. + WidgetsBinding.instance.addPostFrameCallback( + (_) => _stream.removeListener(_listener), + ); + onDone(); + } + + void cancel() { + if (_settled) return; + _settled = true; + _stream.removeListener(_listener); + abandonPendingImage(_stream, provider, config); } } diff --git a/mobile/lib/shared/relay/media_image.dart b/mobile/lib/shared/relay/media_image.dart index c3ae723741..8242d4f0b1 100644 --- a/mobile/lib/shared/relay/media_image.dart +++ b/mobile/lib/shared/relay/media_image.dart @@ -4,6 +4,7 @@ import 'dart:ui' as ui; import 'package:flutter/foundation.dart'; import 'package:flutter/material.dart'; +import 'package:flutter_hooks/flutter_hooks.dart'; import 'package:hooks_riverpod/hooks_riverpod.dart'; import 'package:http/http.dart' as http; @@ -47,9 +48,14 @@ class MediaImageProvider extends ImageProvider { /// Excluded from equality: transport, not identity. final http.Client client; - /// Deadline for the complete response, including a stalled response body. - /// Aborts the request without closing the shared client. - final Duration requestTimeout; + /// Longest wait for response headers or the next body chunk. A slow link + /// that keeps delivering bytes finishes; one that stops is aborted without + /// closing the shared client. + final Duration stallTimeout; + + /// Longest a fetch may take even while bytes keep arriving, so a server + /// that trickles bytes cannot hold a connection indefinitely. + final Duration maxFetchDuration; const MediaImageProvider({ required this.url, @@ -57,7 +63,8 @@ class MediaImageProvider extends ImageProvider { required this.client, this.onBytesLoaded, this.scale = 1.0, - this.requestTimeout = const Duration(seconds: 30), + this.stallTimeout = const Duration(seconds: 20), + this.maxFetchDuration = const Duration(minutes: 5), }); /// Maximum encoded image body retained before decoding (20 MiB). @@ -66,8 +73,7 @@ class MediaImageProvider extends ImageProvider { static const _defaultCooldown = Duration(seconds: 30); static const _maxRetryAfter = Duration(minutes: 5); static final Map _cooldownUntil = {}; - static final Map<(String, MediaGetAuthService), Future> _inFlight = - {}; + static final Map<(String, MediaGetAuthService), _SharedFetch> _inFlight = {}; /// Injectable clock for cooldown tests. @visibleForTesting @@ -89,33 +95,67 @@ class MediaImageProvider extends ImageProvider { MediaImageProvider key, ImageDecoderCallback decode, ) { - return MultiFrameImageStreamCompleter( - codec: _loadAsync(key, decode), + final abandoned = Completer(); + // The image cache listens to a pending load, so this fires mid-download + // only once a consumer evicts the entry and stops listening (see + // [abandonPendingImage]), as [MediaImage] does when it stops showing an + // image before its first frame. Abort the download then. + return _MediaImageStreamCompleter( + codec: _loadAsync(key, decode, abandoned.future), scale: key.scale, debugLabel: url, - ); + )..addOnLastListenerRemovedCallback(() { + if (!abandoned.isCompleted) abandoned.complete(); + }); } Future _loadAsync( MediaImageProvider key, ImageDecoderCallback decode, + Future abandoned, ) async { + var wasAbandoned = false; try { // Each decode size is its own cache entry (see [ResizeImage]), so one // URL shown at two sizes resolves twice. Share the download instead of // fetching the same bytes once per size. final fetchKey = (url, auth); - // whenComplete awaits a returned Future, so the callback must not - // return the removed entry (this same future) or it never completes. - final bytes = await (_inFlight[fetchKey] ??= _fetchBytes().whenComplete( - () { - _inFlight.remove(fetchKey); + final fetch = _inFlight[fetchKey] ??= _SharedFetch( + _fetchBytes, + onFinished: (fetch) { + if (_inFlight[fetchKey] == fetch) _inFlight.remove(fetchKey); }, - )); + ); + fetch.join(); + var left = false; + void leave() { + if (left) return; + left = true; + fetch.leave(); + } + + unawaited( + abandoned.then((_) { + wasAbandoned = true; + leave(); + }), + ); + final Uint8List bytes; + try { + bytes = await fetch.bytes; + } finally { + leave(); + } + // Another size kept the download going, but this load's consumer left. + if (wasAbandoned) throw StateError('Image load abandoned'); onBytesLoaded?.call(bytes); final buffer = await ui.ImmutableBuffer.fromUint8List(bytes); return await decode(buffer); } catch (_) { + // An abandoned load has no listener left to report to, and an error + // would surface as an unhandled image error. The consumer already + // evicted it; leave the codec unresolved. + if (wasAbandoned) return Completer().future; // Match NetworkImage: make sure an errored key is not retained in the // cache. The cooldown map (not the cache) throttles retries. scheduleMicrotask(() { @@ -125,7 +165,9 @@ class MediaImageProvider extends ImageProvider { } } - Future _fetchBytes() async { + /// Downloads [url]. Completing [abort] means every load waiting for this + /// download was abandoned, so the URL is not put on cooldown. + Future _fetchBytes(Future abort) async { final until = _cooldownUntil[url]; if (until != null) { if (debugNow().isBefore(until)) { @@ -136,20 +178,44 @@ class MediaImageProvider extends ImageProvider { final uri = Uri.parse(url); final http.Response response; + var wasAbandoned = false; try { - final abort = Completer(); - final deadline = Timer(requestTimeout, abort.complete); + final stop = Completer(); + Timer? stall; + void restartStallTimer() { + stall?.cancel(); + stall = Timer(stallTimeout, () { + if (!stop.isCompleted) stop.complete(); + }); + } + + restartStallTimer(); + final deadline = Timer(maxFetchDuration, () { + if (!stop.isCompleted) stop.complete(); + }); + unawaited( + abort.then((_) { + if (stop.isCompleted) return; + wasAbandoned = true; + stop.complete(); + }), + ); StreamIterator>? body; try { final request = http.AbortableRequest( 'GET', uri, - abortTrigger: abort.future, + abortTrigger: stop.future, )..headers.addAll(auth.headersFor(url)); final streamed = await client.send(request); + restartStallTimer(); final iterator = body = StreamIterator(streamed.stream); final bytes = BytesBuilder(copy: false); - while (await iterator.moveNext()) { + // An error body is never shown, and reading a slow one could hold + // this load for the whole fetch cap before the fallback or + // Retry-After applies. Leave it unread; finally cancels the stream. + while (streamed.statusCode == 200 && await iterator.moveNext()) { + restartStallTimer(); final chunk = iterator.current; if (chunk.length > maxResponseBytes - bytes.length) { throw const FormatException('Image response too large'); @@ -162,12 +228,16 @@ class MediaImageProvider extends ImageProvider { headers: streamed.headers, ); } finally { + stall?.cancel(); deadline.cancel(); - if (!abort.isCompleted) abort.complete(); + if (!stop.isCompleted) stop.complete(); unawaited(body?.cancel()); } } catch (_) { - _cooldownUntil[url] = debugNow().add(_defaultCooldown); + // Nobody wanted the image any more; the URL itself did not fail. + if (!wasAbandoned) { + _cooldownUntil[url] = debugNow().add(_defaultCooldown); + } rethrow; } if (response.statusCode != 200) { @@ -211,6 +281,95 @@ class MediaImageProvider extends ImageProvider { 'MediaImageProvider("$url", scale: ${scale.toStringAsFixed(1)})'; } +/// Stops waiting for [provider]'s image before its first frame arrives. +/// +/// Call after removing your own listener from [stream]. Until the first frame +/// the image cache holds its own listener on the load, so removing a caller's +/// listener alone leaves the download running. When no other consumer is +/// listening, evicting the pending entry drops that last listener and a +/// [MediaImageProvider] load aborts its download. When another consumer still +/// listens, the entry stays so the finished image is still cached. +void abandonPendingImage( + ImageStream stream, + ImageProvider provider, + ImageConfiguration config, +) { + final completer = stream.completer; + if (completer is _MediaImageStreamCompleter && + completer._listeners.length != 1) { + // Someone besides the cache still wants it, or it already finished. + return; + } + unawaited( + provider + .obtainKey(config) + .then( + (key) => PaintingBinding.instance.imageCache.evict( + key, + includeLive: false, + ), + ), + ); +} + +/// Tracks its listeners so [abandonPendingImage] can tell whether anyone +/// besides the image cache is still waiting for the load. +class _MediaImageStreamCompleter extends MultiFrameImageStreamCompleter { + _MediaImageStreamCompleter({ + required super.codec, + required super.scale, + super.debugLabel, + }); + + final _listeners = []; + + @override + void addListener(ImageStreamListener listener) { + _listeners.add(listener); + super.addListener(listener); + } + + @override + void removeListener(ImageStreamListener listener) { + _listeners.remove(listener); + super.removeListener(listener); + } +} + +/// One download shared by every load of the same URL and auth scope. It is +/// aborted once every load waiting for it has been abandoned, and a load that +/// arrives after that starts a new download. +class _SharedFetch { + _SharedFetch( + Future Function(Future abort) fetch, { + required this.onFinished, + }) { + bytes = fetch(_abort.future).whenComplete(() { + _done = true; + onFinished(this); + }); + // Waiting loads handle errors; an abandoned download has none left. + bytes.ignore(); + } + + final void Function(_SharedFetch) onFinished; + final _abort = Completer(); + late final Future bytes; + var _waiting = 0; + var _done = false; + + void join() => _waiting++; + + /// Called once per joined load, when it is abandoned or has its result. + void leave() { + _waiting--; + if (_waiting == 0 && !_done && !_abort.isCompleted) { + _abort.complete(); + onFinished(this); + } + } +} + /// Thrown when a fetch is suppressed because the URL recently failed. class MediaImageCooldownException implements Exception { final String url; @@ -399,16 +558,55 @@ class MediaImage extends ConsumerWidget { int? cacheWidth, { ImageErrorWidgetBuilder? errorBuilder, }) { - return Image( - image: ResizeImage.resizeIfNeeded(cacheWidth, null, provider), - fit: fit, - width: width, - height: height, - semanticLabel: semanticLabel, - errorBuilder: errorBuilder ?? this.errorBuilder, - frameBuilder: frameBuilder, - filterQuality: filterQuality, - gaplessPlayback: true, + final image = ResizeImage.resizeIfNeeded(cacheWidth, null, provider); + return _AbandonWhenGone( + image: image, + child: Image( + image: image, + fit: fit, + width: width, + height: height, + semanticLabel: semanticLabel, + errorBuilder: errorBuilder ?? this.errorBuilder, + frameBuilder: frameBuilder, + filterQuality: filterQuality, + gaplessPlayback: true, + ), ); } } + +/// Abandons [image]'s download if [child] stops showing it before it loads, +/// for example when a chat image scrolls away. Otherwise the image cache's +/// own listener would keep the download, and its share of a slow link, going +/// until the fetch cap. Another widget still showing the image keeps it. +class _AbandonWhenGone extends HookWidget { + const _AbandonWhenGone({required this.image, required this.child}); + + final ImageProvider image; + final Widget child; + + @override + Widget build(BuildContext context) { + final config = createLocalImageConfiguration(context); + // Resolve only once the child is gone, and only a still-pending load: + // resolving earlier would start the download ahead of the child's + // [ScrollAwareImageProvider], defeating its deferral during a fast fling. + // The microtask runs after this frame, so the child has stopped + // listening and a widget showing the same image elsewhere has started. + useEffect( + () => + () => scheduleMicrotask(() { + final cache = PaintingBinding.instance.imageCache; + unawaited( + image.obtainKey(config).then((key) { + if (!cache.statusForKey(key).pending) return; + abandonPendingImage(image.resolve(config), image, config); + }), + ); + }), + [image], + ); + return child; + } +} diff --git a/mobile/lib/shared/widgets/ios_media_carousel.dart b/mobile/lib/shared/widgets/ios_media_carousel.dart index 19f125a364..1770c6de4d 100644 --- a/mobile/lib/shared/widgets/ios_media_carousel.dart +++ b/mobile/lib/shared/widgets/ios_media_carousel.dart @@ -7,6 +7,8 @@ import 'package:flutter/material.dart'; import 'package:flutter/services.dart'; import 'package:flutter_hooks/flutter_hooks.dart'; +import '../relay/media_image.dart'; + /// UIKit's horizontally scrolling photo strip, using the app's image providers. /// Authentication and image-cache isolation remain owned by Flutter. class IosMediaCarousel extends HookWidget { @@ -129,9 +131,9 @@ class IosMediaCarousel extends HookWidget { } // Keep native subscriptions and unresolved provider loads bounded together. -// A cancelled load keeps its slot until its first frame/error. MediaImageProvider -// aborts stalled HTTP requests at its deadline, allowing queued cells to progress -// without releasing slots while their transports are still running. +// A load cancelled or disposed before its first frame evicts its pending cache +// entry and stops listening; with no listener left, MediaImageProvider aborts its +// download, so the slot is released at once. class _CarouselImages { _CarouselImages(this.channel); final MethodChannel channel; @@ -159,10 +161,15 @@ class _CarouselImages { image.cancelled = true; image.nextFrame?.dispose(); image.nextFrame = null; - if (image.receivedFrame) { - image.detach(); - if (!image.encoding) _finish(image); - } + image.detach(); + _releasePending(image); + if (!image.encoding) _finish(image); + } + + void _releasePending(_CarouselImage image) { + final stream = image.stream; + if (stream == null || image.receivedFrame) return; + abandonPendingImage(stream, image.provider, image.config); } void _drain() { @@ -254,6 +261,8 @@ class _CarouselImages { _queued.clear(); for (final image in _running.values.toList()) { image.cancelled = true; + image.detach(); + _releasePending(image); _finish(image); } } diff --git a/mobile/test/features/channels/message_carousel_preview_cases.dart b/mobile/test/features/channels/message_carousel_preview_cases.dart index 9ab476de19..d7ff26a374 100644 --- a/mobile/test/features/channels/message_carousel_preview_cases.dart +++ b/mobile/test/features/channels/message_carousel_preview_cases.dart @@ -29,12 +29,26 @@ void carouselPreviewTests() { } return null; }); - final client = MockClient((request) { + final aborted = {}; + final client = MockClient.streaming((request, _) async { final url = request.url.toString(); started.add(url); final response = responses[url] = Completer(); - - return response.future; + if (request case http.Abortable(:final abortTrigger?)) { + unawaited( + abortTrigger.then((_) { + // The provider also fires the trigger after a finished load. + if (response.isCompleted) return; + aborted.add(url); + response.completeError(http.RequestAbortedException(request.url)); + }), + ); + } + final body = await response.future; + return http.StreamedResponse( + Stream.value(body.bodyBytes), + body.statusCode, + ); }); Future waitForCount(int count) async { for ( @@ -98,7 +112,59 @@ void carouselPreviewTests() { } await tester.pumpAndSettle(); expect(started.toSet(), urls.take(5).toSet()); - // No native subscribeImage or page callback was sent by this harness. + Future page(int index) async { + await messenger.handlePlatformMessage( + channels.single.name, + channels.single.codec.encodeMethodCall(MethodCall('page', index)), + (_) {}, + ); + await tester.pump(); + await tester.pump(); + } + + // Jumping to the end prefetches around the new page. + await page(9); + await waitForCount(7); + expect(started.skip(5), [urls[9], urls[8]]); + Future native(String method, Object argument) async { + await tester.runAsync( + () => messenger.handlePlatformMessage( + channels.single.name, + channels.single.codec.encodeMethodCall( + MethodCall(method, argument), + ), + (_) {}, + ), + ); + await tester.pump(); + } + + // A native cell that cancels and comes back while the prefetch is still + // loading joins that download instead of starting another. + await native('subscribeImage', {'id': 'a', 'index': 9}); + await native('cancelImage', 'a'); + await native('subscribeImage', {'id': 'b', 'index': 9}); + for (var attempt = 0; attempt < 10; attempt++) { + await tester.pump(); + await tester.runAsync( + () => Future.delayed(const Duration(milliseconds: 10)), + ); + } + expect(started, hasLength(7)); + expect(aborted, isEmpty); + await native('cancelImage', 'b'); + // Leaving before they finish abandons both downloads and frees the + // workers for the new window. + await page(0); + for (var attempt = 0; attempt < 100 && aborted.length < 2; attempt++) { + await tester.pump(); + await tester.runAsync( + () => Future.delayed(const Duration(milliseconds: 10)), + ); + } + expect(aborted, {urls[9], urls[8]}); + await waitForCount(9); + expect(started.skip(7), [urls[0], urls[1]]); await tester.pumpWidget(const SizedBox()); expect(tester.takeException(), isNull); }, diff --git a/mobile/test/shared/relay/media_image_test.dart b/mobile/test/shared/relay/media_image_test.dart index b5a7253086..291139f844 100644 --- a/mobile/test/shared/relay/media_image_test.dart +++ b/mobile/test/shared/relay/media_image_test.dart @@ -4,8 +4,9 @@ import 'dart:typed_data'; import 'package:buzz/shared/relay/media_auth.dart'; import 'package:buzz/shared/relay/media_image.dart'; -import 'package:flutter/painting.dart'; +import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:hooks_riverpod/hooks_riverpod.dart'; import 'package:http/http.dart' as http; import 'package:http/testing.dart' as http_testing; import 'package:nostr/nostr.dart' as nostr; @@ -213,6 +214,80 @@ void main() { expect(fetches, 1); }); + test( + 'abandoning a load another consumer still waits for keeps it cached', + () async { + var fetches = 0; + var aborted = false; + final response = Completer(); + final client = _StreamingClient((request) async { + fetches += 1; + (request as http.AbortableRequest).abortTrigger!.then((_) { + if (!response.isCompleted) aborted = true; + }); + await response.future; + return http.StreamedResponse(Stream.value(_pngBytes), 200); + }); + final provider = MediaImageProvider( + url: _mediaUrl, + auth: _auth(), + client: client, + ); + const config = ImageConfiguration.empty; + final cell = provider.resolve(config); + final cellListener = ImageStreamListener((_, _) {}); + cell.addListener(cellListener); + final prefetch = provider.resolve(config); + final loaded = Completer(); + final prefetchListener = ImageStreamListener((image, _) { + image.dispose(); + if (!loaded.isCompleted) loaded.complete(); + }); + prefetch.addListener(prefetchListener); + + // The cell scrolls away while the prefetch still wants the image. + cell.removeListener(cellListener); + abandonPendingImage(cell, provider, config); + response.complete(); + await loaded.future.timeout(const Duration(seconds: 2)); + prefetch.removeListener(prefetchListener); + + expect(aborted, isFalse); + expect( + PaintingBinding.instance.imageCache.containsKey(provider), + isTrue, + ); + await _wait(provider.resolve(config)); + expect(fetches, 1); + }, + ); + + test('abandoning a load nobody else waits for aborts it', () async { + final aborted = Completer(); + final client = _StreamingClient((request) async { + (request as http.AbortableRequest).abortTrigger!.then((_) { + if (!aborted.isCompleted) aborted.complete(); + }); + await aborted.future; + throw http.RequestAbortedException(request.url); + }); + final provider = MediaImageProvider( + url: _mediaUrl, + auth: _auth(), + client: client, + ); + final stream = provider.resolve(ImageConfiguration.empty); + final listener = ImageStreamListener((_, _) {}); + stream.addListener(listener); + stream.removeListener(listener); + abandonPendingImage(stream, provider, ImageConfiguration.empty); + await aborted.future.timeout(const Duration(seconds: 2)); + expect( + PaintingBinding.instance.imageCache.containsKey(provider), + isFalse, + ); + }); + test('one URL shown at two decode sizes downloads once', () async { var fetches = 0; final response = Completer(); @@ -236,6 +311,53 @@ void main() { expect(fetches, 1); }); + test( + 'a download shared by two sizes outlives one abandoned size', + () async { + var fetches = 0; + var aborts = 0; + final response = Completer(); + final client = _StreamingClient((request) async { + fetches += 1; + final trigger = (request as http.AbortableRequest).abortTrigger!; + unawaited( + trigger.then((_) { + if (!response.isCompleted) aborts += 1; + }), + ); + await Future.any([response.future, trigger]); + if (!response.isCompleted) { + throw http.RequestAbortedException(request.url); + } + return http.StreamedResponse(Stream.value(_pngBytes), 200); + }); + final auth = _auth(nsec: nostr.Keys.generate().nsec); + final base = MediaImageProvider( + url: _mediaUrl, + auth: auth, + client: client, + ); + const config = ImageConfiguration.empty; + final smallProvider = ResizeImage(base, width: 40); + final largeProvider = ResizeImage(base, width: 120); + final small = smallProvider.resolve(config); + final smallListener = ImageStreamListener((_, _) {}); + small.addListener(smallListener); + final largeDone = _wait(largeProvider.resolve(config)); + await pumpEventQueue(); + + small.removeListener(smallListener); + abandonPendingImage(small, smallProvider, config); + await pumpEventQueue(); + expect(aborts, 0); + response.complete(); + await largeDone.timeout(const Duration(seconds: 2)); + expect(fetches, 1); + expect(aborts, 0); + expect(MediaImageProvider.debugCooldownUrls, isEmpty); + }, + ); + test('sends auth headers with the fetch', () async { Map? seen; final client = http_testing.MockClient((request) async { @@ -252,6 +374,68 @@ void main() { expect(seen?['Authorization'], startsWith('Nostr ')); }); + test( + 'a slow body that keeps arriving outlives the stall timeout', + () async { + const gap = Duration(milliseconds: 60); + final client = _StreamingClient((request) async { + // Honor the abort trigger the way the real IO client does. + var aborted = false; + (request as http.AbortableRequest).abortTrigger!.then((_) { + aborted = true; + }); + Stream> trickle() async* { + for (var i = 0; i < _pngBytes.length; i += 12) { + await Future.delayed(gap); + if (aborted) throw http.RequestAbortedException(request.url); + yield _pngBytes.sublist(i, (i + 12).clamp(0, _pngBytes.length)); + } + } + + return http.StreamedResponse(trickle(), 200); + }); + final stopwatch = Stopwatch()..start(); + await _wait( + MediaImageProvider( + url: _mediaUrl, + auth: _auth(), + client: client, + stallTimeout: gap * 2.5, + ).resolve(ImageConfiguration.empty), + ).timeout(const Duration(seconds: 5)); + // The whole body took several stall windows; only gaps are bounded. + expect(stopwatch.elapsed, greaterThan(gap * 5)); + }, + ); + + test('a body that never ends is aborted at the fetch cap', () async { + final client = _StreamingClient((request) async { + var aborted = false; + (request as http.AbortableRequest).abortTrigger!.then((_) { + aborted = true; + }); + Stream> endless() async* { + while (true) { + await Future.delayed(const Duration(milliseconds: 20)); + if (aborted) throw http.RequestAbortedException(request.url); + yield const [0]; + } + } + + return http.StreamedResponse(endless(), 200); + }); + final error = await _waitError( + MediaImageProvider( + url: _mediaUrl, + auth: _auth(), + client: client, + stallTimeout: const Duration(seconds: 5), + maxFetchDuration: const Duration(milliseconds: 200), + ).resolve(ImageConfiguration.empty), + ).timeout(const Duration(seconds: 3)); + expect(error, isA()); + }); + test('failed URL is not refetched until cooldown elapses', () async { var fetches = 0; final client = http_testing.MockClient((request) async { @@ -284,40 +468,179 @@ void main() { expect(fetches, 2); }); - test('honors Retry-After on 429 (capped)', () async { - var fetches = 0; - final client = http_testing.MockClient((request) async { - fetches += 1; - return http.Response( - 'rate limited', - 429, - headers: {'retry-after': '120'}, + test( + 'honors Retry-After on 429 (capped) without reading the body', + () async { + var fetches = 0; + // A body that never ends: the error must not wait for it. + Stream> endless() async* { + while (true) { + await Future.delayed(const Duration(milliseconds: 20)); + yield const [0]; + } + } + + final client = _StreamingClient((request) async { + fetches += 1; + return http.StreamedResponse( + endless(), + 429, + headers: {'retry-after': '120'}, + ); + }); + final auth = _auth(nsec: nostr.Keys.generate().nsec); + var current = DateTime.utc(2026, 7, 21, 12); + MediaImageProvider.debugNow = () => current; + + Future attempt() async { + final provider = MediaImageProvider( + url: _mediaUrl, + auth: auth, + client: client, + ); + return _waitError(provider.resolve(ImageConfiguration.empty)); + } + + final error = await attempt().timeout(const Duration(seconds: 3)); + expect(error, isA()); + expect(fetches, 1); + + current = current.add(const Duration(seconds: 60)); + expect(await attempt(), isA()); + expect(fetches, 1); + + current = current.add(const Duration(seconds: 61)); + await attempt(); + expect(fetches, 2); + }, + ); + }); + + group('stalls', () { + // Each row stalls at a different point; both must abort at the stall + // timeout, well before the fetch cap, and put the URL on cooldown. + final rows = + < + String, + Future Function( + http.BaseRequest request, + Future aborted, + ) + >{ + 'headers never arrive': (request, aborted) async { + await aborted; + throw http.RequestAbortedException(request.url); + }, + 'the body stops partway': (request, aborted) async { + Stream> body() async* { + yield _pngBytes.sublist(0, 8); + await aborted; + throw http.RequestAbortedException(request.url); + } + + return http.StreamedResponse(body(), 200); + }, + }; + for (final MapEntry(key: name, value: respond) in rows.entries) { + test('a silent connection is aborted when $name', () async { + final client = _StreamingClient( + (request) => respond( + request, + (request as http.AbortableRequest).abortTrigger!, + ), ); + final stopwatch = Stopwatch()..start(); + final error = await _waitError( + MediaImageProvider( + url: _mediaUrl, + auth: _auth(), + client: client, + stallTimeout: const Duration(milliseconds: 150), + maxFetchDuration: const Duration(seconds: 30), + ).resolve(ImageConfiguration.empty), + ).timeout(const Duration(seconds: 5)); + expect(error, isA()); + expect(stopwatch.elapsed, lessThan(const Duration(seconds: 5))); + expect(MediaImageProvider.debugCooldownUrls, [_mediaUrl]); }); - final auth = _auth(nsec: nostr.Keys.generate().nsec); - var current = DateTime.utc(2026, 7, 21, 12); - MediaImageProvider.debugNow = () => current; + } + }); - Future attempt() async { - final provider = MediaImageProvider( - url: _mediaUrl, - auth: auth, - client: client, - ); - return _waitError(provider.resolve(ImageConfiguration.empty)); - } - - await attempt(); - expect(fetches, 1); - - current = current.add(const Duration(seconds: 60)); - expect(await attempt(), isA()); - expect(fetches, 1); - - current = current.add(const Duration(seconds: 61)); - await attempt(); - expect(fetches, 2); + testWidgets('an image that leaves before it loads stops downloading, ' + 'unless another widget still shows it', (tester) async { + var aborted = false; + var requests = 0; + final client = _StreamingClient((request) async { + requests += 1; + // A slow link: the body has started but does not finish in this test. + final body = StreamController>()..add(const [0]); + (request as http.AbortableRequest).abortTrigger!.then((_) { + aborted = true; + body + ..addError(http.RequestAbortedException(request.url)) + ..close(); + }); + return http.StreamedResponse(body.stream, 200); }); + final shown = ValueNotifier(2); + addTearDown(shown.dispose); + // A fast fling: the scrollable asks images to defer loading. + final flinging = ValueNotifier(true); + addTearDown(flinging.dispose); + await tester.pumpWidget( + ProviderScope( + overrides: [ + mediaGetAuthServiceProvider.overrideWithValue(_auth()), + mediaHttpClientProvider.overrideWithValue(client), + ], + child: MaterialApp( + home: ListenableBuilder( + listenable: Listenable.merge([shown, flinging]), + builder: (_, _) => SingleChildScrollView( + physics: flinging.value ? const _DeferringPhysics() : null, + child: Column( + children: [ + for (var i = 0; i < shown.value; i++) + const SizedBox.square( + dimension: 40, + child: MediaImage(url: _mediaUrl), + ), + ], + ), + ), + ), + ), + ), + ); + Future settle() async { + for (var i = 0; i < 3; i++) { + await tester.runAsync( + () => Future.delayed(const Duration(milliseconds: 50)), + ); + await tester.pump(); + } + } + + await settle(); + // Nothing downloads while the fling defers loading. + expect(requests, 0); + + flinging.value = false; + await settle(); + // Both copies share one download. + expect(requests, 1); + + // One copy scrolls away; the other still shows the image. + shown.value = 1; + await settle(); + expect(aborted, isFalse); + + // The last one leaves: the download stops, and the URL itself did not + // fail, so it is not put on cooldown. + shown.value = 0; + await settle(); + expect(aborted, isTrue); + expect(MediaImageProvider.debugCooldownUrls, isEmpty); }); } @@ -355,3 +678,19 @@ class _StreamingClient extends http.BaseClient { Future send(http.BaseRequest request) => respond(request); } + +/// Physics that always recommend deferring image loads, as during a fling. +class _DeferringPhysics extends ScrollPhysics { + const _DeferringPhysics({super.parent}); + + @override + _DeferringPhysics applyTo(ScrollPhysics? ancestor) => + _DeferringPhysics(parent: buildParent(ancestor)); + + @override + bool recommendDeferredLoading( + double velocity, + ScrollMetrics metrics, + BuildContext context, + ) => true; +} diff --git a/mobile/test/shared/widgets/ios_media_carousel_test.dart b/mobile/test/shared/widgets/ios_media_carousel_test.dart index 9990c0c3b9..3184ca3a19 100644 --- a/mobile/test/shared/widgets/ios_media_carousel_test.dart +++ b/mobile/test/shared/widgets/ios_media_carousel_test.dart @@ -16,7 +16,7 @@ import 'package:flutter_test/flutter_test.dart'; void main() { for (final stallBody in [false, true]) { testWidgets( - 'stalled ${stallBody ? "bodies" : "headers"} abort before queued photo loads', + 'previews scrolled past abort their downloads while ${stallBody ? "bodies" : "headers"} stall', (tester) async { final client = (await tester.runAsync(() async => _TrackingClient()))!; final server = await tester.runAsync( @@ -39,7 +39,8 @@ void main() { request.response.add([0]); unawaited(request.response.flush()); } - // The first four never finish: only client-side abort can release them. + // The first four never finish and never time out within the + // test: only the carousel's cancellation can release them. }); }); final harness = await _mount( @@ -50,7 +51,7 @@ void main() { url: '$base/$index', auth: auth, client: client, - requestTimeout: const Duration(seconds: 1), + stallTimeout: const Duration(minutes: 1), ), ), ); @@ -62,15 +63,18 @@ void main() { }); await harness.send('cancelImage', '$index'); } - await harness.send('subscribeImage', {'id': 'latest', 'index': 4}); - expect(client.started, 4); - await harness.waitForFrames(1); await client.retired.future.timeout(const Duration(seconds: 5)); + await harness.send('subscribeImage', {'id': 'latest', 'index': 4}); + await harness.waitForFrames(1); + // Scrolling back starts a fresh download: an abandoned load is not + // a failure, so it leaves no cooldown behind. + await harness.send('subscribeImage', {'id': 'back', 'index': 0}); + for (var i = 0; i < 100 && client.started < 6; i++) { + await Future.delayed(const Duration(milliseconds: 10)); + } }); - expect(client.started, 5); - expect(client.peak, 4); expect(client.aborted, 4); - expect(client.active, 0); + expect(client.started, 6); expect(harness.frames, [ {'id': 'latest', 'bytes': null}, ]); @@ -80,6 +84,50 @@ void main() { ); } + testWidgets('closing the carousel aborts its pending downloads', ( + tester, + ) async { + final client = (await tester.runAsync(() async => _TrackingClient()))!; + final server = await tester.runAsync( + () => HttpServer.bind(InternetAddress.loopbackIPv4, 0), + ); + addTearDown(() async { + client.close(); + await server!.close(force: true); + MediaImageProvider.debugResetCooldowns(); + }); + final base = 'http://127.0.0.1:${server!.port}'; + final auth = MediaGetAuthService(baseUrl: base, nsec: null); + // Requests never get a response: only disposal can release them. + await tester.runAsync(() async => server.listen((_) {})); + final harness = await _mount( + tester, + List.generate( + 4, + (index) => MediaImageProvider( + url: '$base/$index', + auth: auth, + client: client, + stallTimeout: const Duration(minutes: 1), + ), + ), + ); + await tester.runAsync(() async { + for (var index = 0; index < 4; index++) { + await harness.send('subscribeImage', {'id': '$index', 'index': index}); + } + for (var i = 0; i < 100 && client.started < 4; i++) { + await Future.delayed(const Duration(milliseconds: 10)); + } + }); + expect(client.started, 4); + await tester.pumpWidget(const SizedBox()); + await tester.runAsync( + () => client.retired.future.timeout(const Duration(seconds: 5)), + ); + expect(client.aborted, 4); + }, variant: TargetPlatformVariant.only(TargetPlatform.iOS)); + testWidgets('native callbacks and disposal release subscribed images', ( tester, ) async { @@ -145,37 +193,32 @@ void main() { expect(still.completer.listeners, isEmpty); }, variant: TargetPlatformVariant.only(TargetPlatform.iOS)); - testWidgets( - 'rapid reuse bounds unresolved loads and drops cancelled queued requests', - (tester) async { - final providers = List.generate(20, (_) => _TestImage()); - final harness = await _mount(tester, providers); - for (var index = 0; index < 20; index++) { - await harness.send('subscribeImage', {'id': '$index', 'index': index}); - if (index < 19) await harness.send('cancelImage', '$index'); - } - expect(providers.where((p) => p.resolutions > 0), hasLength(4)); - expect( - providers.skip(4).take(15).every((p) => p.completer.listeners.isEmpty), - isTrue, - ); - // Retired pending providers retain a bounded completion observer because - // transport termination must precede releasing that slot. - final image = await tester.runAsync(() => _frame(Colors.red)); - providers[0].completer.emit(image!); - await tester.pump(); - expect(providers[0].completer.listeners, isEmpty); - expect(providers[19].resolutions, 1); - expect( - harness.frames, - isEmpty, - reason: 'Cancelled loads must never reach UIKit', - ); - await tester.pumpWidget(const SizedBox()); - expect(providers.every((p) => p.completer.listeners.isEmpty), isTrue); - }, - variant: TargetPlatformVariant.only(TargetPlatform.iOS), - ); + testWidgets('rapid reuse releases cancelled loads at once', (tester) async { + final providers = List.generate(20, (_) => _TestImage()); + final harness = await _mount(tester, providers); + for (var index = 0; index < 20; index++) { + await harness.send('subscribeImage', {'id': '$index', 'index': index}); + if (index < 19) await harness.send('cancelImage', '$index'); + } + // A cancelled load stops listening, which lets MediaImageProvider abort + // its download, so it never holds a slot the visible cell needs. + expect(providers.every((p) => p.resolutions == 1), isTrue); + expect( + providers.take(19).every((p) => p.completer.listeners.isEmpty), + isTrue, + ); + expect(providers[19].completer.listeners, hasLength(1)); + final image = await tester.runAsync(() => _frame(Colors.red)); + providers[0].completer.emit(image!); + await tester.pump(); + expect( + harness.frames, + isEmpty, + reason: 'Cancelled loads must never reach UIKit', + ); + await tester.pumpWidget(const SizedBox()); + expect(providers.every((p) => p.completer.listeners.isEmpty), isTrue); + }, variant: TargetPlatformVariant.only(TargetPlatform.iOS)); } Future _frame(Color color) async {