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.
pinterest · May 26, 2017 · 659ffa7 · 659ffa7
1 parent 5ea785e
commit 659ffa7
Show file tree

Hide file tree

Showing 6 changed files with 165 additions and 18 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,6 @@
 ## master
 * Add your own contributions to the next release on the line below this with your name.
+- [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)
 - [fixed] Fixes an issue where PINResume would assert because the server didn't return an expected content length.
 - [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)
 - [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)

diff --git a/Source/Classes/Categories/NSURLSessionTask+Timing.m b/Source/Classes/Categories/NSURLSessionTask+Timing.m
@@ -11,6 +11,8 @@
 #import <objc/runtime.h>
 #import <QuartzCore/QuartzCore.h>
 
+static NSString * const kPINURLSessionTaskStateKey = @"state";
+
 @interface PINURLSessionTaskObserver : NSObject
 
 @property (atomic, assign) CFTimeInterval startTime;
@@ -28,30 +30,38 @@ - (instancetype)initWithTask:(NSURLSessionTask *)task
     if (self = [super init]) {
         _startTime = 0;
         _endTime = 0;
-        [task addObserver:self forKeyPath:@"state" options:0 context:nil];
+        [task addObserver:self forKeyPath:kPINURLSessionTaskStateKey options:0 context:nil];
     }
     return self;
 }
 
+- (void)removeObservers:(NSURLSessionTask *)task
+{
+    [task removeObserver:self forKeyPath:kPINURLSessionTaskStateKey];
+}
+
+
 - (void)observeValueForKeyPath:(NSString *)keyPath ofObject:(id)object change:(NSDictionary<NSKeyValueChangeKey,id> *)change context:(void *)context
 {
     NSURLSessionTask *task = (NSURLSessionTask *)object;
-    switch (task.state) {
-        case NSURLSessionTaskStateRunning:
-            if (self.startTime == 0) {
-                self.startTime = CACurrentMediaTime();
-            }
-            break;
-
-        case NSURLSessionTaskStateCompleted:
-            NSAssert(self.startTime != 0, @"Expect that task was started before it's completed.");
-            if (self.endTime == 0) {
-                self.endTime = CACurrentMediaTime();
-            }
-            break;
-
-        default:
-            break;
+    if ([keyPath isEqualToString:kPINURLSessionTaskStateKey]) {
+        switch (task.state) {
+            case NSURLSessionTaskStateRunning:
+                if (self.startTime == 0) {
+                    self.startTime = CACurrentMediaTime();
+                }
+                break;
+
+            case NSURLSessionTaskStateCompleted:
+                NSAssert(self.startTime > 0, @"Expect that task was started before it's completed.");
+                if (self.endTime == 0) {
+                    self.endTime = CACurrentMediaTime();
+                }
+                break;
+
+            default:
+                break;
+        }
     }
 }
 
@@ -74,7 +84,7 @@ - (void)PIN_setupSessionTaskObserver
                 //remove state observer
                 PINURLSessionTaskObserver *observer = objc_getAssociatedObject((__bridge id)obj, @selector(PIN_setupSessionTaskObserver));
                 if (observer) {
-                    [(__bridge id)obj removeObserver:observer forKeyPath:@"state"];
+                    [observer removeObservers:(__bridge NSURLSessionTask *)obj];
                 }
             }
 

diff --git a/Source/Classes/PINRemoteImageDownloadTask.m b/Source/Classes/PINRemoteImageDownloadTask.m
@@ -96,6 +96,16 @@ - (BOOL)cancelWithUUID:(NSUUID *)UUID resume:(PINResume * _Nullable * _Nullable)
 {
     __block BOOL noMoreCompletions;
     [self.lock lockWithBlock:^{
+        if (resume) {
+            //consider skipping cancelation if there's a request for resume data and the time to start the connection is greater than
+            //the time remaining to download.
+            NSTimeInterval timeToFirstByte = [self.manager.sessionManager weightedTimeToFirstByteForHost:_progressImage.dataTask.originalRequest.URL.host];
+            if (_progressImage.estimatedRemainingTime <= timeToFirstByte) {
+                noMoreCompletions = NO;
+                return;
+            }
+        }
+
         noMoreCompletions = [super l_cancelWithUUID:UUID resume:resume];
 
         if (noMoreCompletions) {

diff --git a/Source/Classes/PINURLSessionManager.h b/Source/Classes/PINURLSessionManager.h
@@ -35,6 +35,14 @@ typedef void (^PINURLSessionDataTaskCompletion)(NSURLSessionTask * _Nonnull task
 
 @property (atomic, weak, nullable) id <PINURLSessionManagerDelegate> delegate;
 
+/*
+ Returns a weighted average of time to first byte for the specified host.
+ More specifically, we get the time to first byte for every task that completes
+ and add it to an existing average: newAverage = (existingAverage + newTimeToFirstByte / 2)
+ This is all done on a per host basis.
+ */
+- (NSTimeInterval)weightedTimeToFirstByteForHost:(nonnull NSString *)host;
+
 #if DEBUG
 - (void)concurrentDownloads:(void (^_Nullable)(NSUInteger concurrentDownloads))concurrentDownloadsCompletion;
 #endif

diff --git a/Source/Classes/PINURLSessionManager.m b/Source/Classes/PINURLSessionManager.m
@@ -11,6 +11,9 @@
 NSString * const PINURLErrorDomain = @"PINURLErrorDomain";
 
 @interface PINURLSessionManager () <NSURLSessionDelegate, NSURLSessionDataDelegate>
+{
+    NSCache *_timeToFirstByteCache;
+}
 
 @property (nonatomic, strong) NSLock *sessionManagerLock;
 @property (nonatomic, strong) NSURLSession *session;
@@ -35,6 +38,9 @@ - (instancetype)initWithSessionConfiguration:(NSURLSessionConfiguration *)config
         self.session = [NSURLSession sessionWithConfiguration:configuration delegate:self delegateQueue:self.operationQueue];
         self.completions = [[NSMutableDictionary alloc] init];
         self.delegateQueues = [[NSMutableDictionary alloc] init];
+
+        _timeToFirstByteCache = [[NSCache alloc] init];
+        _timeToFirstByteCache.countLimit = 25;
     }
     return self;
 }
@@ -187,6 +193,53 @@ - (void)URLSession:(NSURLSession *)session task:(NSURLSessionTask *)task didComp
     });
 }
 
+- (void)URLSession:(NSURLSession *)session task:(NSURLSessionTask *)task didFinishCollectingMetrics:(NSURLSessionTaskMetrics *)metrics
+{
+    NSDate *requestStart = [NSDate distantFuture];
+    NSDate *firstByte = [NSDate distantPast];
+
+    for (NSURLSessionTaskTransactionMetrics *metric in metrics.transactionMetrics) {
+        if (metric.requestStartDate == nil || metric.responseStartDate == nil) {
+            //Only evaluate requests which completed their first byte.
+            return;
+        }
+        if ([requestStart compare:metric.requestStartDate] != NSOrderedAscending) {
+            requestStart = metric.requestStartDate;
+        }
+        if ([firstByte compare:metric.responseStartDate] != NSOrderedDescending) {
+            firstByte = metric.responseStartDate;
+        }
+    }
+
+    [self storeTimeToFirstByte:[firstByte timeIntervalSinceDate:requestStart] forHost:task.originalRequest.URL.host];
+}
+
+/* We don't bother locking around the timeToFirstByteCache because NSCache itself is
+ threadsafe and we're not concerned about dropping or overwriting a result. */
+- (void)storeTimeToFirstByte:(NSTimeInterval)timeToFirstByte forHost:(NSString *)host
+{
+    NSNumber *existingTimeToFirstByte = [_timeToFirstByteCache objectForKey:host];
+    if (existingTimeToFirstByte) {
+        //We're obviously seriously weighting the latest result by doing this. Seems reasonable in
+        //possibly changing network conditions.
+        existingTimeToFirstByte = @( (timeToFirstByte + [existingTimeToFirstByte doubleValue]) / 2.0 );
+    } else {
+        existingTimeToFirstByte = [NSNumber numberWithDouble:timeToFirstByte];
+    }
+    [_timeToFirstByteCache setObject:existingTimeToFirstByte forKey:host];
+}
+
+- (NSTimeInterval)weightedTimeToFirstByteForHost:(NSString *)host
+{
+    NSTimeInterval timeToFirstByte;
+    timeToFirstByte = [[_timeToFirstByteCache objectForKey:host] doubleValue];
+    if (timeToFirstByte <= 0 + DBL_EPSILON) {
+        //return 0 if we're not sure.
+        timeToFirstByte = 0;
+    }
+    return timeToFirstByte;
+}
+
 #if DEBUG
 - (void)concurrentDownloads:(void (^_Nullable)(NSUInteger concurrentDownloads))concurrentDownloadsCompletion
 {

diff --git a/Tests/PINRemoteImageTests.m b/Tests/PINRemoteImageTests.m
@@ -47,6 +47,7 @@ - (NSString *)resumeCacheKeyForURL:(NSURL *)url;
 @interface PINURLSessionManager ()
 
 @property (nonatomic, strong) NSURLSession *session;
+- (void)storeTimeToFirstByte:(NSTimeInterval)timeToFirstByte forHost:(NSString *)host;
 
 @end
 #endif
@@ -257,6 +258,10 @@ - (void)testCustomRequestHeaderIsAddedToImageRequests
     [self.imageManager setValue:@"Custom Request Header 2" forHTTPHeaderField:@"X-Custom-Request-Header-2"];
     [self.imageManager setValue:nil forHTTPHeaderField:@"X-Custom-Request-Header-2"];
     self.imageManager.sessionManager.delegate = self;
+
+    //sleep for a moment so values have a chance to asynchronously set.
+    usleep(10000);
+
     [self.imageManager downloadImageWithURL:[self progressiveURL]
                                     options:PINRemoteImageManagerDownloadOptionsNone
                                  completion:^(PINRemoteImageManagerResult *result)
@@ -1162,4 +1167,64 @@ - (void)testResume
     XCTAssert([nonResumedImageData isEqualToData:resumedImageData], @"Resumed image data and non resumed image data should be the same.");
 }
 
+- (void)testResumeSkipCancelation
+{
+    //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)
+    dispatch_semaphore_t semaphore = dispatch_semaphore_create(0);
+
+    weakify(self);
+    [self.imageManager setEstimatedRemainingTimeThresholdForProgressiveDownloads:0.001 completion:^{
+        strongify(self);
+        [self.imageManager setProgressiveRendersMaxProgressiveRenderSize:CGSizeMake(10000, 10000) completion:^{
+            dispatch_semaphore_signal(semaphore);
+        }];
+    }];
+    dispatch_semaphore_wait(semaphore, [self timeout]);
+
+    XCTestExpectation *progressExpectation = [self expectationWithDescription:@"progress is rendered"];
+
+    [self.imageManager.sessionManager storeTimeToFirstByte:0 forHost:[[self progressiveURL] host]];
+
+    __block BOOL canceled = NO;
+    [self.imageManager downloadImageWithURL:[self progressiveURL]
+                                    options:PINRemoteImageManagerDownloadOptionsNone
+                              progressImage:^(PINRemoteImageManagerResult * _Nonnull result) {
+                                  if (canceled == NO) {
+                                      canceled = YES;
+                                      [self.imageManager cancelTaskWithUUID:result.UUID storeResumeData:YES];
+                                      [progressExpectation fulfill];
+                                      dispatch_semaphore_signal(semaphore);
+                                  }
+                              }
+                                 completion:^(PINRemoteImageManagerResult * _Nonnull result) {
+                                     XCTAssert(result.image == nil, @"should not complete download: %@", result);
+                                 }];
+
+    dispatch_semaphore_wait(semaphore, [self timeout]);
+
+    //Remove any progress
+    [self.imageManager.cache removeObjectForKey:[self.imageManager resumeCacheKeyForURL:[self progressiveURL]]];
+
+    XCTestExpectation *progress2Expectation = [self expectationWithDescription:@"progress 2 is rendered"];
+    XCTestExpectation *completedExpectation = [self expectationWithDescription:@"image is completed"];
+
+    [self.imageManager.sessionManager storeTimeToFirstByte:10 forHost:[[self progressiveURL] host]];
+
+    canceled = NO;
+    [self.imageManager downloadImageWithURL:[self progressiveURL]
+                                    options:PINRemoteImageManagerDownloadOptionsNone
+                              progressImage:^(PINRemoteImageManagerResult * _Nonnull result) {
+                                  if (canceled == NO) {
+                                      canceled = YES;
+                                      [self.imageManager cancelTaskWithUUID:result.UUID storeResumeData:YES];
+                                      [progress2Expectation fulfill];
+                                  }
+                              }
+                                 completion:^(PINRemoteImageManagerResult * _Nonnull result) {
+                                     [completedExpectation fulfill];
+                                 }];
+
+    [self waitForExpectationsWithTimeout:[self timeoutTimeInterval] handler:nil];
+}
+
 @end