Skip to content

Commit 659ffa7

Browse files
garrettmoongarrettmoon
authored
Adding support for automatically skipping cancelation if the amount l… (#364)
* Adding support for automatically skipping cancelation if the amount left to be downloaded is less than the average time to first byte. * Cleaning up unused properties * More missed stuff left in * Added CHANGELOG * Remove assertForOverFulfill, which is bizarly not supported? * Addressing @jparise's comments.
1 parent 5ea785e commit 659ffa7

File tree

6 files changed

+165
-18
lines changed

6 files changed

+165
-18
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
## master
22
* Add your own contributions to the next release on the line below this with your name.
3+
- [new] Added support (in iOS 10) for skipping cancelation if the estimated amount of time to complete the download is less than the average time to first byte for a host. [#364](https://github.com/pinterest/PINRemoteImage/pull/364) [garrettmoon](http://github.com/garrettmoon)
34
- [fixed] Fixes an issue where PINResume would assert because the server didn't return an expected content length.
45
- [fixed] Fixed bytes per second on download tasks (which could affect if an image is progressively rendered) [#360](https://github.com/pinterest/PINRemoteImage/pull/360) [garrettmoon](https://github.com/garrettmoon)
56
- [new] Added request configuration handler to allow customizing HTTP headers per request [#355](https://github.com/pinterest/PINRemoteImage/pull/355) [zachwaugh](https://github.com/zachwaugh)

‎Source/Classes/Categories/NSURLSessionTask+Timing.m

Lines changed: 28 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@
1111
#import <objc/runtime.h>
1212
#import <QuartzCore/QuartzCore.h>
1313

14+
static NSString * const kPINURLSessionTaskStateKey = @"state";
15+
1416
@interface PINURLSessionTaskObserver : NSObject
1517

1618
@property (atomic, assign) CFTimeInterval startTime;
@@ -28,30 +30,38 @@ - (instancetype)initWithTask:(NSURLSessionTask *)task
2830
if (self = [super init]) {
2931
_startTime = 0;
3032
_endTime = 0;
31-
[task addObserver:self forKeyPath:@"state" options:0 context:nil];
33+
[task addObserver:self forKeyPath:kPINURLSessionTaskStateKey options:0 context:nil];
3234
}
3335
return self;
3436
}
3537

38+
- (void)removeObservers:(NSURLSessionTask *)task
39+
{
40+
[task removeObserver:self forKeyPath:kPINURLSessionTaskStateKey];
41+
}
42+
43+
3644
- (void)observeValueForKeyPath:(NSString *)keyPath ofObject:(id)object change:(NSDictionary<NSKeyValueChangeKey,id> *)change context:(void *)context
3745
{
3846
NSURLSessionTask *task = (NSURLSessionTask *)object;
39-
switch (task.state) {
40-
case NSURLSessionTaskStateRunning:
41-
if (self.startTime == 0) {
42-
self.startTime = CACurrentMediaTime();
43-
}
44-
break;
45-
46-
case NSURLSessionTaskStateCompleted:
47-
NSAssert(self.startTime != 0, @"Expect that task was started before it's completed.");
48-
if (self.endTime == 0) {
49-
self.endTime = CACurrentMediaTime();
50-
}
51-
break;
52-
53-
default:
54-
break;
47+
if ([keyPath isEqualToString:kPINURLSessionTaskStateKey]) {
48+
switch (task.state) {
49+
case NSURLSessionTaskStateRunning:
50+
if (self.startTime == 0) {
51+
self.startTime = CACurrentMediaTime();
52+
}
53+
break;
54+
55+
case NSURLSessionTaskStateCompleted:
56+
NSAssert(self.startTime > 0, @"Expect that task was started before it's completed.");
57+
if (self.endTime == 0) {
58+
self.endTime = CACurrentMediaTime();
59+
}
60+
break;
61+
62+
default:
63+
break;
64+
}
5565
}
5666
}
5767

@@ -74,7 +84,7 @@ - (void)PIN_setupSessionTaskObserver
7484
//remove state observer
7585
PINURLSessionTaskObserver *observer = objc_getAssociatedObject((__bridge id)obj, @selector(PIN_setupSessionTaskObserver));
7686
if (observer) {
77-
[(__bridge id)obj removeObserver:observer forKeyPath:@"state"];
87+
[observer removeObservers:(__bridge NSURLSessionTask *)obj];
7888
}
7989
}
8090

‎Source/Classes/PINRemoteImageDownloadTask.m

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,16 @@ - (BOOL)cancelWithUUID:(NSUUID *)UUID resume:(PINResume * _Nullable * _Nullable)
9696
{
9797
__block BOOL noMoreCompletions;
9898
[self.lock lockWithBlock:^{
99+
if (resume) {
100+
//consider skipping cancelation if there's a request for resume data and the time to start the connection is greater than
101+
//the time remaining to download.
102+
NSTimeInterval timeToFirstByte = [self.manager.sessionManager weightedTimeToFirstByteForHost:_progressImage.dataTask.originalRequest.URL.host];
103+
if (_progressImage.estimatedRemainingTime <= timeToFirstByte) {
104+
noMoreCompletions = NO;
105+
return;
106+
}
107+
}
108+
99109
noMoreCompletions = [super l_cancelWithUUID:UUID resume:resume];
100110

101111
if (noMoreCompletions) {

‎Source/Classes/PINURLSessionManager.h

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,14 @@ typedef void (^PINURLSessionDataTaskCompletion)(NSURLSessionTask * _Nonnull task
3535

3636
@property (atomic, weak, nullable) id <PINURLSessionManagerDelegate> delegate;
3737

38+
/*
39+
Returns a weighted average of time to first byte for the specified host.
40+
More specifically, we get the time to first byte for every task that completes
41+
and add it to an existing average: newAverage = (existingAverage + newTimeToFirstByte / 2)
42+
This is all done on a per host basis.
43+
*/
44+
- (NSTimeInterval)weightedTimeToFirstByteForHost:(nonnull NSString *)host;
45+
3846
#if DEBUG
3947
- (void)concurrentDownloads:(void (^_Nullable)(NSUInteger concurrentDownloads))concurrentDownloadsCompletion;
4048
#endif

‎Source/Classes/PINURLSessionManager.m

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,9 @@
1111
NSString * const PINURLErrorDomain = @"PINURLErrorDomain";
1212

1313
@interface PINURLSessionManager () <NSURLSessionDelegate, NSURLSessionDataDelegate>
14+
{
15+
NSCache *_timeToFirstByteCache;
16+
}
1417

1518
@property (nonatomic, strong) NSLock *sessionManagerLock;
1619
@property (nonatomic, strong) NSURLSession *session;
@@ -35,6 +38,9 @@ - (instancetype)initWithSessionConfiguration:(NSURLSessionConfiguration *)config
3538
self.session = [NSURLSession sessionWithConfiguration:configuration delegate:self delegateQueue:self.operationQueue];
3639
self.completions = [[NSMutableDictionary alloc] init];
3740
self.delegateQueues = [[NSMutableDictionary alloc] init];
41+
42+
_timeToFirstByteCache = [[NSCache alloc] init];
43+
_timeToFirstByteCache.countLimit = 25;
3844
}
3945
return self;
4046
}
@@ -187,6 +193,53 @@ - (void)URLSession:(NSURLSession *)session task:(NSURLSessionTask *)task didComp
187193
});
188194
}
189195

196+
- (void)URLSession:(NSURLSession *)session task:(NSURLSessionTask *)task didFinishCollectingMetrics:(NSURLSessionTaskMetrics *)metrics
197+
{
198+
NSDate *requestStart = [NSDate distantFuture];
199+
NSDate *firstByte = [NSDate distantPast];
200+
201+
for (NSURLSessionTaskTransactionMetrics *metric in metrics.transactionMetrics) {
202+
if (metric.requestStartDate == nil || metric.responseStartDate == nil) {
203+
//Only evaluate requests which completed their first byte.
204+
return;
205+
}
206+
if ([requestStart compare:metric.requestStartDate] != NSOrderedAscending) {
207+
requestStart = metric.requestStartDate;
208+
}
209+
if ([firstByte compare:metric.responseStartDate] != NSOrderedDescending) {
210+
firstByte = metric.responseStartDate;
211+
}
212+
}
213+
214+
[self storeTimeToFirstByte:[firstByte timeIntervalSinceDate:requestStart] forHost:task.originalRequest.URL.host];
215+
}
216+
217+
/* We don't bother locking around the timeToFirstByteCache because NSCache itself is
218+
threadsafe and we're not concerned about dropping or overwriting a result. */
219+
- (void)storeTimeToFirstByte:(NSTimeInterval)timeToFirstByte forHost:(NSString *)host
220+
{
221+
NSNumber *existingTimeToFirstByte = [_timeToFirstByteCache objectForKey:host];
222+
if (existingTimeToFirstByte) {
223+
//We're obviously seriously weighting the latest result by doing this. Seems reasonable in
224+
//possibly changing network conditions.
225+
existingTimeToFirstByte = @( (timeToFirstByte + [existingTimeToFirstByte doubleValue]) / 2.0 );
226+
} else {
227+
existingTimeToFirstByte = [NSNumber numberWithDouble:timeToFirstByte];
228+
}
229+
[_timeToFirstByteCache setObject:existingTimeToFirstByte forKey:host];
230+
}
231+
232+
- (NSTimeInterval)weightedTimeToFirstByteForHost:(NSString *)host
233+
{
234+
NSTimeInterval timeToFirstByte;
235+
timeToFirstByte = [[_timeToFirstByteCache objectForKey:host] doubleValue];
236+
if (timeToFirstByte <= 0 + DBL_EPSILON) {
237+
//return 0 if we're not sure.
238+
timeToFirstByte = 0;
239+
}
240+
return timeToFirstByte;
241+
}
242+
190243
#if DEBUG
191244
- (void)concurrentDownloads:(void (^_Nullable)(NSUInteger concurrentDownloads))concurrentDownloadsCompletion
192245
{

‎Tests/PINRemoteImageTests.m

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ - (NSString *)resumeCacheKeyForURL:(NSURL *)url;
4747
@interface PINURLSessionManager ()
4848

4949
@property (nonatomic, strong) NSURLSession *session;
50+
- (void)storeTimeToFirstByte:(NSTimeInterval)timeToFirstByte forHost:(NSString *)host;
5051

5152
@end
5253
#endif
@@ -257,6 +258,10 @@ - (void)testCustomRequestHeaderIsAddedToImageRequests
257258
[self.imageManager setValue:@"Custom Request Header 2" forHTTPHeaderField:@"X-Custom-Request-Header-2"];
258259
[self.imageManager setValue:nil forHTTPHeaderField:@"X-Custom-Request-Header-2"];
259260
self.imageManager.sessionManager.delegate = self;
261+
262+
//sleep for a moment so values have a chance to asynchronously set.
263+
usleep(10000);
264+
260265
[self.imageManager downloadImageWithURL:[self progressiveURL]
261266
options:PINRemoteImageManagerDownloadOptionsNone
262267
completion:^(PINRemoteImageManagerResult *result)
@@ -1162,4 +1167,64 @@ - (void)testResume
11621167
XCTAssert([nonResumedImageData isEqualToData:resumedImageData], @"Resumed image data and non resumed image data should be the same.");
11631168
}
11641169

1170+
- (void)testResumeSkipCancelation
1171+
{
1172+
//Test that images aren't canceled if the cost of resuming is high (i.e. time to first byte is longer than the time left to download)
1173+
dispatch_semaphore_t semaphore = dispatch_semaphore_create(0);
1174+
1175+
weakify(self);
1176+
[self.imageManager setEstimatedRemainingTimeThresholdForProgressiveDownloads:0.001 completion:^{
1177+
strongify(self);
1178+
[self.imageManager setProgressiveRendersMaxProgressiveRenderSize:CGSizeMake(10000, 10000) completion:^{
1179+
dispatch_semaphore_signal(semaphore);
1180+
}];
1181+
}];
1182+
dispatch_semaphore_wait(semaphore, [self timeout]);
1183+
1184+
XCTestExpectation *progressExpectation = [self expectationWithDescription:@"progress is rendered"];
1185+
1186+
[self.imageManager.sessionManager storeTimeToFirstByte:0 forHost:[[self progressiveURL] host]];
1187+
1188+
__block BOOL canceled = NO;
1189+
[self.imageManager downloadImageWithURL:[self progressiveURL]
1190+
options:PINRemoteImageManagerDownloadOptionsNone
1191+
progressImage:^(PINRemoteImageManagerResult * _Nonnull result) {
1192+
if (canceled == NO) {
1193+
canceled = YES;
1194+
[self.imageManager cancelTaskWithUUID:result.UUID storeResumeData:YES];
1195+
[progressExpectation fulfill];
1196+
dispatch_semaphore_signal(semaphore);
1197+
}
1198+
}
1199+
completion:^(PINRemoteImageManagerResult * _Nonnull result) {
1200+
XCTAssert(result.image == nil, @"should not complete download: %@", result);
1201+
}];
1202+
1203+
dispatch_semaphore_wait(semaphore, [self timeout]);
1204+
1205+
//Remove any progress
1206+
[self.imageManager.cache removeObjectForKey:[self.imageManager resumeCacheKeyForURL:[self progressiveURL]]];
1207+
1208+
XCTestExpectation *progress2Expectation = [self expectationWithDescription:@"progress 2 is rendered"];
1209+
XCTestExpectation *completedExpectation = [self expectationWithDescription:@"image is completed"];
1210+
1211+
[self.imageManager.sessionManager storeTimeToFirstByte:10 forHost:[[self progressiveURL] host]];
1212+
1213+
canceled = NO;
1214+
[self.imageManager downloadImageWithURL:[self progressiveURL]
1215+
options:PINRemoteImageManagerDownloadOptionsNone
1216+
progressImage:^(PINRemoteImageManagerResult * _Nonnull result) {
1217+
if (canceled == NO) {
1218+
canceled = YES;
1219+
[self.imageManager cancelTaskWithUUID:result.UUID storeResumeData:YES];
1220+
[progress2Expectation fulfill];
1221+
}
1222+
}
1223+
completion:^(PINRemoteImageManagerResult * _Nonnull result) {
1224+
[completedExpectation fulfill];
1225+
}];
1226+
1227+
[self waitForExpectationsWithTimeout:[self timeoutTimeInterval] handler:nil];
1228+
}
1229+
11651230
@end

0 commit comments

Comments
 (0)