fix(images): release artwork slots on non-200 responses
This commit is contained in:
@@ -6,6 +6,7 @@ import 'package:cached_network_image_ce/cached_network_image.dart' show FileResp
|
||||
// behind a narrower unsupported-platform stub.
|
||||
// ignore: implementation_imports
|
||||
import 'package:cached_network_image_ce/src/cache/default_cache_manager.dart' as ce_cache;
|
||||
import 'package:flutter/foundation.dart' show visibleForTesting;
|
||||
import 'package:http/http.dart' as http;
|
||||
import 'package:path_provider/path_provider.dart';
|
||||
|
||||
@@ -33,7 +34,7 @@ class PlexImageCacheManager extends ce_cache.DefaultCacheManager {
|
||||
: super(
|
||||
stalePeriod: const Duration(days: 14),
|
||||
maxNrOfCacheObjects: 3000,
|
||||
httpClientFactory: () => _SharedHttpClient(_artworkHttpClient.inner),
|
||||
httpClientFactory: () => _SharedHttpClient(_artworkHttpClient.inner, _artworkRequestLimiter),
|
||||
cacheDirectoryProvider: getApplicationCacheDirectory,
|
||||
);
|
||||
|
||||
@@ -57,12 +58,13 @@ class PlexImageCacheManager extends ce_cache.DefaultCacheManager {
|
||||
/// transferring ownership of its lifecycle, and cap artwork fan-out globally.
|
||||
class _SharedHttpClient extends http.BaseClient {
|
||||
final http.Client _inner;
|
||||
final _RequestLimiter _limiter;
|
||||
|
||||
_SharedHttpClient(this._inner);
|
||||
_SharedHttpClient(this._inner, this._limiter);
|
||||
|
||||
@override
|
||||
Future<http.StreamedResponse> send(http.BaseRequest request) async {
|
||||
final permit = await _artworkRequestLimiter.acquire();
|
||||
final permit = await _limiter.acquire();
|
||||
var released = false;
|
||||
|
||||
void release() {
|
||||
@@ -73,6 +75,29 @@ class _SharedHttpClient extends http.BaseClient {
|
||||
|
||||
try {
|
||||
final response = await _inner.send(request);
|
||||
|
||||
// CE's cache manager throws for any status other than 200/202 without
|
||||
// listening to the body, so _releaseWhenDone would never fire and the
|
||||
// permit would leak; six stale-thumb 404s then wedge all artwork loading
|
||||
// until restart (#1473). Release now, drain the (tiny) error body in the
|
||||
// background so the platform client reclaims the connection, and hand CE
|
||||
// an empty body it never reads anyway. Status set mirrors CE 4.6.4
|
||||
// _downloadFile; recheck if the pinned dep is ever bumped.
|
||||
if (response.statusCode != 200 && response.statusCode != 202) {
|
||||
release();
|
||||
unawaited(response.stream.drain<void>().catchError((_) {}));
|
||||
return http.StreamedResponse(
|
||||
const Stream<List<int>>.empty(),
|
||||
response.statusCode,
|
||||
contentLength: 0,
|
||||
request: response.request,
|
||||
headers: response.headers,
|
||||
isRedirect: response.isRedirect,
|
||||
persistentConnection: response.persistentConnection,
|
||||
reasonPhrase: response.reasonPhrase,
|
||||
);
|
||||
}
|
||||
|
||||
return http.StreamedResponse(
|
||||
_releaseWhenDone(response.stream, release),
|
||||
response.statusCode,
|
||||
@@ -93,6 +118,11 @@ class _SharedHttpClient extends http.BaseClient {
|
||||
void close() {}
|
||||
}
|
||||
|
||||
/// Test hook: builds the throttled artwork client with an isolated limiter.
|
||||
@visibleForTesting
|
||||
http.Client createArtworkHttpClientForTest(http.Client inner, {int maxConcurrent = 6}) =>
|
||||
_SharedHttpClient(inner, _RequestLimiter(maxConcurrent));
|
||||
|
||||
Stream<List<int>> _releaseWhenDone(Stream<List<int>> stream, void Function() release) async* {
|
||||
try {
|
||||
await for (final chunk in stream) {
|
||||
|
||||
@@ -0,0 +1,104 @@
|
||||
import 'dart:async';
|
||||
|
||||
import 'package:flutter_test/flutter_test.dart';
|
||||
import 'package:http/http.dart' as http;
|
||||
import 'package:http/testing.dart';
|
||||
import 'package:plezy/services/image_cache_service.dart';
|
||||
|
||||
/// Pins the artwork request limiter's permit lifecycle. CE's cache manager
|
||||
/// abandons non-200/202 responses without listening to the body, so the
|
||||
/// wrapper must release those slots itself or the limiter wedges after a
|
||||
/// handful of stale-thumb 404s (#1473).
|
||||
void main() {
|
||||
const timeout = Duration(seconds: 5);
|
||||
|
||||
http.Request get(String path) => http.Request('GET', Uri.parse('https://example.invalid$path'));
|
||||
|
||||
MockClient mixedClient() => MockClient((request) async {
|
||||
if (request.url.path.contains('missing')) {
|
||||
return http.Response('not found', 404, headers: {'x-marker': 'err'});
|
||||
}
|
||||
return http.Response('poster-bytes', 200);
|
||||
});
|
||||
|
||||
test('error burst does not wedge the limiter (#1473)', () async {
|
||||
final client = createArtworkHttpClientForTest(mixedClient(), maxConcurrent: 2);
|
||||
|
||||
// More failures than permits, bodies deliberately never read — mirrors
|
||||
// CE throwing on the status without listening to the stream.
|
||||
for (var i = 0; i < 5; i++) {
|
||||
final response = await client.send(get('/missing/$i')).timeout(timeout);
|
||||
expect(response.statusCode, 404);
|
||||
expect(response.headers['x-marker'], 'err');
|
||||
}
|
||||
|
||||
final ok = await client.send(get('/poster')).timeout(timeout);
|
||||
expect(ok.statusCode, 200);
|
||||
expect(await ok.stream.bytesToString().timeout(timeout), 'poster-bytes');
|
||||
});
|
||||
|
||||
test('successful downloads still hold a slot until the body is drained', () async {
|
||||
final client = createArtworkHttpClientForTest(mixedClient(), maxConcurrent: 1);
|
||||
|
||||
final first = await client.send(get('/poster')).timeout(timeout);
|
||||
var secondDone = false;
|
||||
final second = client.send(get('/poster')).then((response) async {
|
||||
secondDone = true;
|
||||
await response.stream.drain<void>();
|
||||
});
|
||||
|
||||
await pumpEventQueue();
|
||||
expect(secondDone, isFalse, reason: 'slot must stay held while the first body is unread');
|
||||
|
||||
await first.stream.drain<void>();
|
||||
await second.timeout(timeout);
|
||||
expect(secondDone, isTrue);
|
||||
});
|
||||
|
||||
test('error bodies are drained so the transport can reclaim the connection', () async {
|
||||
final listened = Completer<void>();
|
||||
final body = StreamController<List<int>>(onListen: () => listened.complete());
|
||||
unawaited(body.close());
|
||||
final client = createArtworkHttpClientForTest(
|
||||
MockClient.streaming((request, _) async => http.StreamedResponse(body.stream, 404)),
|
||||
maxConcurrent: 2,
|
||||
);
|
||||
|
||||
final response = await client.send(get('/missing')).timeout(timeout);
|
||||
expect(response.statusCode, 404);
|
||||
await listened.future.timeout(timeout);
|
||||
});
|
||||
|
||||
test('a throwing send releases its slot', () async {
|
||||
var failures = 0;
|
||||
final client = createArtworkHttpClientForTest(
|
||||
MockClient((request) async {
|
||||
if (failures < 2) {
|
||||
failures++;
|
||||
throw http.ClientException('boom');
|
||||
}
|
||||
return http.Response('poster-bytes', 200);
|
||||
}),
|
||||
maxConcurrent: 1,
|
||||
);
|
||||
|
||||
for (var i = 0; i < 2; i++) {
|
||||
await expectLater(client.send(get('/poster')).timeout(timeout), throwsA(isA<http.ClientException>()));
|
||||
}
|
||||
|
||||
final ok = await client.send(get('/poster')).timeout(timeout);
|
||||
expect(ok.statusCode, 200);
|
||||
});
|
||||
|
||||
test('cancelling a successful body releases its slot', () async {
|
||||
final client = createArtworkHttpClientForTest(mixedClient(), maxConcurrent: 1);
|
||||
|
||||
final first = await client.send(get('/poster')).timeout(timeout);
|
||||
final subscription = first.stream.listen((_) {});
|
||||
await subscription.cancel();
|
||||
|
||||
final ok = await client.send(get('/poster')).timeout(timeout);
|
||||
expect(ok.statusCode, 200);
|
||||
await ok.stream.drain<void>();
|
||||
});
|
||||
}
|
||||
Reference in New Issue
Block a user