Merge pull request #48 from thomasvl/docs_n_threading

Comment doc updates and threading fixup

Author: thomasvl

Source/GTMMIMEDocument.m

@@ -318,16 +318,14 @@ + (NSData *)dataWithHeaders:(NSDictionary *)headers {
 
 #if DEBUG
     // Look for troublesome characters in the header keys & values.
-    static NSCharacterSet *badChars = nil;
-    if (!badChars) {
-      badChars = [NSCharacterSet characterSetWithCharactersInString:@":\r\n"];
-    }
+    NSCharacterSet *badKeyChars = [NSCharacterSet characterSetWithCharactersInString:@":\r\n"];
+    NSCharacterSet *badValueChars = [NSCharacterSet characterSetWithCharactersInString:@"\r\n"];
 
-    NSRange badRange = [key rangeOfCharacterFromSet:badChars];
-    NSAssert1(badRange.location == NSNotFound, @"invalid key: %@", key);
+    NSRange badRange = [key rangeOfCharacterFromSet:badKeyChars];
+    NSAssert(badRange.location == NSNotFound, @"invalid key: %@", key);
 
-    badRange = [value rangeOfCharacterFromSet:badChars];
-    NSAssert1(badRange.location == NSNotFound, @"invalid value: %@", value);
+    badRange = [value rangeOfCharacterFromSet:badValueChars];
+    NSAssert(badRange.location == NSNotFound, @"invalid value: %@", value);
 #endif
 
     [headerString appendFormat:@"%@: %@\r\n", key, value];

Source/GTMSessionFetcher.h

@@ -329,6 +329,13 @@
   #endif  // __has_feature(nullability)
 #endif  // GTM_NULLABLE
 
+#if ((!TARGET_OS_IPHONE && defined(MAC_OS_X_VERSION_10_12) && MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_12) \
+      || (TARGET_OS_IPHONE && defined(__IPHONE_10_0) && __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_10_0))
+#define GTMSESSION_DEPRECATE_ON_2016_SDKS __attribute__((deprecated))
+#else
+#define GTMSESSION_DEPRECATE_ON_2016_SDKS
+#endif
+
 #ifndef GTM_DECLARE_GENERICS
   #if __has_feature(objc_generics) \
     && ((!TARGET_OS_IPHONE && defined(MAC_OS_X_VERSION_10_11) && MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_11) \
@@ -707,20 +714,40 @@ NSData * GTM_NULLABLE_TYPE GTMDataFromInputStream(NSInputStream *inputStream, NS
 + (GTM_NSArrayOf(GTMSessionFetcher *) *)fetchersForBackgroundSessions;
 
 // Designated initializer.
+//
+// Applications should create fetchers with a "fetcherWith..." method on a fetcher
+// service or a class method, not with this initializer.
+//
+// The configuration should typically be nil. Applications needing to customize
+// the configuration may do so by setting the configurationBlock property.
 - (instancetype)initWithRequest:(GTM_NULLABLE NSURLRequest *)request
                   configuration:(GTM_NULLABLE NSURLSessionConfiguration *)configuration;
 
-// The fetcher's request
+// The fetcher's request.  This may not be set after beginFetch has been invoked. The request
+// may change due to redirects.
+@property(strong, GTM_NULLABLE) NSURLRequest *request;
+
+// Set a header field value on the request. Header field value changes will not
+// affect a fetch after the fetch has begun.
+- (void)setRequestValue:(GTM_NULLABLE NSString *)value forHTTPHeaderField:(NSString *)field;
+
+// The fetcher's request (deprecated.)
 //
-// The underlying request is mutable and may be modified by the caller.  Request changes will not
-// affect a fetch after it has begun.
-@property(readonly, GTM_NULLABLE) NSMutableURLRequest *mutableRequest;
+// Exposing a mutable object in the interface was convenient but a bad design decision due
+// to thread-safety requirements.  Clients should use the request property and
+// setRequestValue:forHTTPHeaderField: instead.
+@property(readonly, GTM_NULLABLE) NSMutableURLRequest *mutableRequest GTMSESSION_DEPRECATE_ON_2016_SDKS;
 
 // Data used for resuming a download task.
 @property(readonly, GTM_NULLABLE) NSData *downloadResumeData;
 
 // The configuration; this must be set before the fetch begins. If no configuration is
 // set or inherited from the fetcher service, then the fetcher uses an ephemeral config.
+//
+// NOTE: This property should typically be nil. Applications needing to customize
+// the configuration should do so by setting the configurationBlock property.
+// That allows the fetcher to pick an appropriate base configuration, with the
+// application setting only the configuration properties it needs to customize.
 @property(strong, GTM_NULLABLE) NSURLSessionConfiguration *configuration;
 
 // A block the client may use to customize the configuration used to create the session.

Source/GTMSessionFetcher.m

@@ -80,7 +80,6 @@
 @interface GTMSessionFetcher ()
 
 @property(strong, readwrite, GTM_NULLABLE) NSData *downloadedData;
-@property(strong, readwrite, GTM_NULLABLE) NSMutableURLRequest *mutableRequest;
 @property(strong, readwrite, GTM_NULLABLE) NSData *downloadResumeData;
 
 #if GTM_BACKGROUND_TASK_FETCHING
@@ -118,7 +117,7 @@ static BOOL IsLocalhost(NSString * GTM_NULLABLE_TYPE host) {
 static GTMSessionFetcherTestBlock GTM_NULLABLE_TYPE gGlobalTestBlock;
 
 @implementation GTMSessionFetcher {
-  NSMutableURLRequest *_request; // may change due to redirects
+  NSMutableURLRequest *_request; // after beginFetch, changed only in delegate callbacks
   BOOL _useUploadTask;           // immutable after beginFetch
   NSURL *_bodyFileURL;           // immutable after beginFetch
   GTMSessionFetcherBodyStreamProvider _bodyStreamProvider;  // immutable after beginFetch
@@ -322,7 +321,7 @@ - (id)copyWithZone:(NSZone *)zone {
 }
 
 - (NSString *)description {
-  NSString *requestStr = self.mutableRequest.URL.description;
+  NSString *requestStr = self.request.URL.description;
   if (requestStr.length == 0) {
     if (self.downloadResumeData.length > 0) {
       requestStr = @"<download resume data>";
@@ -351,6 +350,8 @@ - (void)dealloc {
 // for the duration of the fetch connection.
 
 - (void)beginFetchWithCompletionHandler:(GTM_NULLABLE GTMSessionFetcherCompletionHandler)handler {
+  GTMSessionCheckNotSynchronized(self);
+
   _completionHandler = [handler copy];
 
   // The user may have called setDelegate: earlier if they want to use other
@@ -381,6 +382,8 @@ - (GTMSessionFetcherCompletionHandler)completionHandlerWithTarget:(GTM_NULLABLE_
 
 - (void)beginFetchWithDelegate:(GTM_NULLABLE_TYPE id)target
              didFinishSelector:(GTM_NULLABLE_TYPE SEL)finishedSelector {
+  GTMSessionCheckNotSynchronized(self);
+
   GTMSessionFetcherCompletionHandler handler =  [self completionHandlerWithTarget:target
                                                                 didFinishSelector:finishedSelector];
   [self beginFetchWithCompletionHandler:handler];
@@ -389,8 +392,9 @@ - (void)beginFetchWithDelegate:(GTM_NULLABLE_TYPE id)target
 - (void)beginFetchMayDelay:(BOOL)mayDelay
               mayAuthorize:(BOOL)mayAuthorize {
   // This is the internal entry point for re-starting fetches.
+  GTMSessionCheckNotSynchronized(self);
 
-  NSMutableURLRequest *fetchRequest = self.mutableRequest;
+  NSMutableURLRequest *fetchRequest = _request;  // The request property is now externally immutable.
   NSURL *fetchRequestURL = fetchRequest.URL;
   NSString *priorSessionIdentifier = self.sessionIdentifier;
 
@@ -671,19 +675,20 @@ - (void)beginFetchMayDelay:(BOOL)mayDelay
   BOOL needsUploadTask = (self.useUploadTask || self.bodyFileURL || self.bodyStreamProvider);
   if (_bodyData || self.bodyStreamProvider || fetchRequest.HTTPBodyStream) {
     if (isEffectiveHTTPGet) {
-      [fetchRequest setHTTPMethod:@"POST"];
+      fetchRequest.HTTPMethod = @"POST";
       isEffectiveHTTPGet = NO;
     }
 
     if (_bodyData) {
       if (!needsUploadTask) {
-        [fetchRequest setHTTPBody:_bodyData];
+        fetchRequest.HTTPBody = _bodyData;
       }
 #if !STRIP_GTM_FETCH_LOGGING
     } else if (fetchRequest.HTTPBodyStream) {
       if ([self respondsToSelector:@selector(loggedInputStreamForInputStream:)]) {
-        fetchRequest.HTTPBodyStream = [self performSelector:@selector(loggedInputStreamForInputStream:)
-                                                 withObject:fetchRequest.HTTPBodyStream];
+        fetchRequest.HTTPBodyStream =
+            [self performSelector:@selector(loggedInputStreamForInputStream:)
+                       withObject:fetchRequest.HTTPBodyStream];
       }
 #endif
     }
@@ -1502,7 +1507,8 @@ - (void)authorizeRequest {
   SEL asyncAuthSel = @selector(authorizeRequest:delegate:didFinishSelector:);
   if ([authorizer respondsToSelector:asyncAuthSel]) {
     SEL callbackSel = @selector(authorizer:request:finishedWithError:);
-    [authorizer authorizeRequest:self.mutableRequest
+    NSMutableURLRequest *mutableRequest = [self.request mutableCopy];
+    [authorizer authorizeRequest:mutableRequest
                         delegate:self
                didFinishSelector:callbackSel];
   } else {
@@ -1516,12 +1522,17 @@ - (void)authorizeRequest {
 }
 
 - (void)authorizer:(id<GTMFetcherAuthorizationProtocol>)auth
-           request:(NSMutableURLRequest *)request
+           request:(NSMutableURLRequest *)authorizedRequest
  finishedWithError:(NSError *)error {
+  GTMSessionCheckNotSynchronized(self);
+
   if (error != nil) {
     // We can't fetch without authorization
     [self failToBeginFetchWithError:error];
   } else {
+    @synchronized(self) {
+      _request = authorizedRequest;
+    }
     [self beginFetchMayDelay:NO
                 mayAuthorize:NO];
   }
@@ -1932,7 +1943,7 @@ - (void)URLSession:(NSURLSession *)session
   }
   if (redirectRequest && redirectResponse) {
     // Copy the original request, including the body.
-    NSMutableURLRequest *originalRequest = self.mutableRequest;
+    NSURLRequest *originalRequest = self.request;
     NSMutableURLRequest *newRequest = [originalRequest mutableCopy];
 
     // Disallow scheme changes (say, from https to http).
@@ -2624,11 +2635,13 @@ - (void)URLSession:(NSURLSession *)session
   // header.
   if ((status == 403) && self.usingBackgroundSession) {
     NSURL *redirectURL = self.response.URL;
-    NSMutableURLRequest *request = self.mutableRequest;
+    NSURLRequest *request = self.request;
     if (![request.URL isEqual:redirectURL]) {
       NSString *authorizationHeader = [request.allHTTPHeaderFields objectForKey:@"Authorization"];
       if (authorizationHeader != nil) {
-        request.URL = redirectURL;
+        NSMutableURLRequest *mutableRequest = [request mutableCopy];
+        mutableRequest.URL = redirectURL;
+        [self updateMutableRequest:mutableRequest];
         // Avoid assuming the session is still valid.
         self.session = nil;
         forceAssumeRetry = YES;
@@ -2904,7 +2917,7 @@ - (void)shouldRetryNowForStatus:(NSInteger)status
   if (hasPrimed) {
     shouldRetryForAuthRefresh = YES;
     _hasAttemptedAuthRefresh = YES;
-    [self.mutableRequest setValue:nil forHTTPHeaderField:@"Authorization"];
+    [self updateRequestValue:nil forHTTPHeaderField:@"Authorization"];
   }
 
   @synchronized(self) {
@@ -3278,19 +3291,46 @@ + (GTMSessionFetcherSystemCompletionHandler GTM_NULLABLE_TYPE)systemCompletionHa
             skipBackgroundTask = _skipBackgroundTask;
 #endif
 
-- (GTM_NULLABLE NSMutableURLRequest *)mutableRequest {
+- (GTM_NULLABLE NSURLRequest *)request {
   @synchronized(self) {
     GTMSessionMonitorSynchronized(self);
 
-    return _request;
+    return [_request copy];
+  }  // @synchronized(self)
+}
+
+- (void)setRequest:(GTM_NULLABLE NSURLRequest *)request {
+  @synchronized(self) {
+    GTMSessionMonitorSynchronized(self);
+
+    if (![self isFetchingUnsynchronized]) {
+      _request = [request mutableCopy];
+    } else {
+      GTMSESSION_ASSERT_DEBUG(0, @"request may not be set after beginFetch has been invoked");
+    }
   }  // @synchronized(self)
 }
 
-- (GTM_NULLABLE NSMutableURLRequest *)mutableRequestUnsynchronized {
+- (GTM_NULLABLE NSMutableURLRequest *)mutableRequestForTesting {
+  // Allow tests only to modify the request, useful during retries.
   return _request;
 }
 
+- (GTM_NULLABLE NSMutableURLRequest *)mutableRequest {
+  @synchronized(self) {
+    GTMSessionMonitorSynchronized(self);
+
+    GTMSESSION_LOG_DEBUG(@"[GTMSessionFetcher mutableRequest] is deprecated; use -request or"
+                         @" -setRequestVaue:forHTTPHeaderField:");
+
+    return _request;
+  }  // @synchronized(self)
+}
+
 - (void)setMutableRequest:(GTM_NULLABLE NSMutableURLRequest *)request {
+  GTMSESSION_LOG_DEBUG(@"[GTMSessionFetcher setMutableRequest:] is deprecated; use -request or"
+                       @" -setRequestVaue:forHTTPHeaderField:");
+
   GTMSESSION_ASSERT_DEBUG(![self isFetching],
                           @"mutableRequest should not change after beginFetch has been invoked");
   [self updateMutableRequest:request];
@@ -3305,7 +3345,26 @@ - (void)updateMutableRequest:(GTM_NULLABLE NSMutableURLRequest *)request {
   }  // @synchronized(self)
 }
 
-- (void)setResponse:(GTM_NULLABLE NSURLResponse *)response {
+// Set a header field value on the request. Header field value changes will not
+// affect a fetch after the fetch has begun.
+- (void)setRequestValue:(GTM_NULLABLE NSString *)value forHTTPHeaderField:(NSString *)field {
+  if (![self isFetching]) {
+    [self updateRequestValue:value forHTTPHeaderField:field];
+  } else {
+    GTMSESSION_ASSERT_DEBUG(0, @"request may not be set after beginFetch has been invoked");
+  }
+}
+
+// Internal method for updating request headers.
+- (void)updateRequestValue:(GTM_NULLABLE NSString *)value forHTTPHeaderField:(NSString *)field {
+  @synchronized(self) {
+    GTMSessionMonitorSynchronized(self);
+
+    [_request setValue:value forHTTPHeaderField:field];
+  }  // @synchronized(self)
+}
+
+- (void)setResponse:(NSURLResponse * GTM_NULLABLE_TYPE)response {
   @synchronized(self) {
     GTMSessionMonitorSynchronized(self);
 

Source/GTMSessionFetcherLogging.m

@@ -540,7 +540,7 @@ - (void)logFetchWithError:(NSError *)error {
   [outputHTML appendFormat:@"elapsed: %5.3fsec<br>", elapsed];
 
   // write the request URL
-  NSURLRequest *request = self.mutableRequest;
+  NSURLRequest *request = self.request;
   NSString *requestMethod = request.HTTPMethod;
   NSURL *requestURL = request.URL;
 
@@ -595,7 +595,7 @@ - (void)logFetchWithError:(NSError *)error {
   } else {
     bodyData = self.bodyData;
     if (bodyData == nil) {
-      bodyData = self.mutableRequest.HTTPBody;
+      bodyData = self.request.HTTPBody;
     }
   }
   uint64_t bodyDataLength = bodyData.length;

Source/GTMSessionFetcherService.h

@@ -111,6 +111,12 @@ extern NSString *const kGTMSessionFetcherServiceSessionKey;
 - (GTMSessionFetcher *)fetcherWithRequest:(NSURLRequest *)request;
 - (GTMSessionFetcher *)fetcherWithURL:(NSURL *)requestURL;
 - (GTMSessionFetcher *)fetcherWithURLString:(NSString *)requestURLString;
+
+// Common method for fetcher creation.
+//
+// -fetcherWithRequest:fetcherClass: may be overridden to customize creation of
+// fetchers.  This is the ONLY method in the GTMSessionFetcher library intended to
+// be overridden.
 - (id)fetcherWithRequest:(NSURLRequest *)request
             fetcherClass:(Class)fetcherClass;
 
@@ -139,7 +145,7 @@ extern NSString *const kGTMSessionFetcherServiceSessionKey;
 - (GTM_NULLABLE id<NSURLSessionDelegate>)sessionDelegate;
 - (GTM_NULLABLE NSDate *)stoppedAllFetchersDate;
 
-// The testBlock can inspect its fetcher parameter's mutableRequest property to
+// The testBlock can inspect its fetcher parameter's request property to
 // determine which fetcher is being faked.
 @property(copy, GTM_NULLABLE) GTMSessionFetcherTestBlock testBlock;
 

Source/GTMSessionFetcherService.m

@@ -159,6 +159,7 @@ - (void)dealloc {
 
 #pragma mark Generate a new fetcher
 
+// Clients may override this method. Clients should not override any other library methods.
 - (id)fetcherWithRequest:(NSURLRequest *)request
             fetcherClass:(Class)fetcherClass {
   GTMSessionFetcher *fetcher = [[fetcherClass alloc] initWithRequest:request
@@ -191,8 +192,8 @@ - (id)fetcherWithRequest:(NSURLRequest *)request
   NSString *userAgent = self.userAgent;
   if (userAgent.length > 0
       && [request valueForHTTPHeaderField:@"User-Agent"] == nil) {
-    [fetcher.mutableRequest setValue:userAgent
-                  forHTTPHeaderField:@"User-Agent"];
+    [fetcher setRequestValue:userAgent
+          forHTTPHeaderField:@"User-Agent"];
   }
   fetcher.testBlock = self.testBlock;
 
@@ -302,7 +303,7 @@ - (BOOL)isDelayingFetcher:(GTMSessionFetcher *)fetcher {
   @synchronized(self) {
     GTMSessionMonitorSynchronized(self);
 
-    NSString *host = fetcher.mutableRequest.URL.host;
+    NSString *host = fetcher.request.URL.host;
     if (host == nil) {
       return NO;
     }
@@ -315,7 +316,7 @@ - (BOOL)isDelayingFetcher:(GTMSessionFetcher *)fetcher {
 
 - (BOOL)fetcherShouldBeginFetching:(GTMSessionFetcher *)fetcher {
   // Entry point from the fetcher
-  NSURL *requestURL = fetcher.mutableRequest.URL;
+  NSURL *requestURL = fetcher.request.URL;
   NSString *host = requestURL.host;
 
   // Addresses "file:///path" case where localhost is the implicit host.
@@ -578,7 +579,7 @@ - (NSArray *)issuedFetchersWithRequestURL:(NSURL *)requestURL {
   NSIndexSet *indexes = [allFetchers indexesOfObjectsPassingTest:^BOOL(GTMSessionFetcher *fetcher,
                                                                        NSUInteger idx,
                                                                        BOOL *stop) {
-      NSURL *fetcherURL = [fetcher.mutableRequest.URL absoluteURL];
+      NSURL *fetcherURL = [fetcher.request.URL absoluteURL];
       return [fetcherURL isEqual:targetURL];
   }];