Merge pull request google#55 from thomasvl/update

Start addressing race condition/deadlock issue.
welshm · Aug 5, 2016 · 400a467 · 400a467
2 parents bdf2708 + fcbeb9d
commit 400a467
Show file tree

Hide file tree

Showing 2 changed files with 86 additions and 43 deletions.
diff --git a/Source/GTMSessionFetcher.h b/Source/GTMSessionFetcher.h
@@ -86,7 +86,7 @@
 // be uploaded from a system temporary or cache directory.
 //
 // Background session transfers are slower, and should typically be used only
-// or very large downloads or uploads (hundreds of megabytes).
+// for very large downloads or uploads (hundreds of megabytes).
 //
 // When background sessions are used in iOS apps, the application delegate must
 // pass through the parameters from UIApplicationDelegate's
@@ -134,7 +134,7 @@
 // HTTP methods and headers:
 //
 // Alternative HTTP methods, like PUT, and custom headers can be specified by
-// creating the fetcher with an appropriate NSMutableURLRequest
+// creating the fetcher with an appropriate NSMutableURLRequest.
 //
 //
 // Caching:

diff --git a/Source/GTMSessionFetcherService.m b/Source/GTMSessionFetcherService.m
@@ -48,13 +48,12 @@ @interface GTMSessionFetcherSessionDelegateDispatcher : NSObject<NSURLSessionDel
 // The session for the tasks in this dispatcher's task-to-fetcher map.
 @property(atomic) NSURLSession *session;
 
-// This semaphore can block access to the session property while another fetcher is creating
-// a session.
-@property(atomic, readonly) dispatch_semaphore_t sessionCreationSemaphore;
-
 // The timer interval for invalidating a session that has no active tasks.
 @property(atomic) NSTimeInterval discardInterval;
 
+// The current discard timer.
+@property(atomic, readonly) NSTimer *discardTimer;
+
 
 - (instancetype)initWithParentService:(GTMSessionFetcherService *)parentService
                sessionDiscardInterval:(NSTimeInterval)discardInterval;
@@ -81,6 +80,9 @@ @implementation GTMSessionFetcherService {
   // When this ivar is nil, the service will not reuse sessions.
   GTMSessionFetcherSessionDelegateDispatcher *_delegateDispatcher;
 
+  // Fetchers will wait on this if another fetcher is creating the shared NSURLSession.
+  dispatch_semaphore_t _sessionCreationSemaphore;
+
   dispatch_queue_t _callbackQueue;
   NSOperationQueue *_delegateQueue;
   NSHTTPCookieStorage *_cookieStorage;
@@ -141,6 +143,8 @@ - (instancetype)init {
     _callbackQueue = dispatch_get_main_queue();
     _delegateQueue = [NSOperationQueue mainQueue];
 
+    _sessionCreationSemaphore = dispatch_semaphore_create(1);
+
     // Starting with the SDKs for OS X 10.11/iOS 9, the service has a default useragent.
     // Apps can remove this and get the default system "CFNetwork" useragent by setting the
     // fetcher service's userAgent property to nil.
@@ -228,34 +232,32 @@ - (NSURLSession *)session {
 // waits on a semaphore, blocking other fetchers while the caller creates the
 // session if needed.
 - (NSURLSession *)sessionForFetcherCreation {
-  // Avoid waiting in the @synchronized section since that can deadlock.
-  dispatch_semaphore_t semaphore;
   @synchronized(self) {
     GTMSessionMonitorSynchronized(self);
-
-    [_delegateDispatcher startSessionUsage];
-
-    semaphore = _delegateDispatcher.sessionCreationSemaphore;
-    GTMSESSION_ASSERT_DEBUG(semaphore != nil || _delegateDispatcher == nil, @"Expected semaphore");
-  }
-  if (semaphore) {
-    // Wait if another fetcher is currently creating a session.
-    dispatch_semaphore_wait(semaphore, DISPATCH_TIME_FOREVER);
-  } else {
-    // This fetcher is creating a non-shared session, so skip the semaphore usage.
-    return self.session;
+    if (!_delegateDispatcher) {
+      // This fetcher is creating a non-shared session, so skip the semaphore usage.
+      return nil;
+    }
   }
 
+  // Wait if another fetcher is currently creating a session; avoid waiting
+  // inside the @synchronized block, as that can deadlock.
+  dispatch_semaphore_wait(_sessionCreationSemaphore, DISPATCH_TIME_FOREVER);
+
   @synchronized(self) {
     GTMSessionMonitorSynchronized(self);
 
+    // Before getting the NSURLSession for task creation, it is
+    // important to invalidate and nil out the session discard timer; otherwise
+    // the session can be invalidated between when it is returned to the
+    // fetcher, and when the fetcher attempts to create its NSURLSessionTask.
+    [_delegateDispatcher startSessionUsage];
+
     NSURLSession *session = _delegateDispatcher.session;
     if (session) {
-      GTMSESSION_ASSERT_DEBUG(semaphore == _delegateDispatcher.sessionCreationSemaphore,
-                              @"delegate dispatcher semaphore changed");
       // The calling fetcher will receive a preexisting session, so
       // we can allow other fetchers to create a session.
-      dispatch_semaphore_signal(semaphore);
+      dispatch_semaphore_signal(_sessionCreationSemaphore);
     } else {
       // No existing session was obtained, so the calling fetcher will create the session;
       // it *must* invoke fetcherDidCreateSession: to signal the dispatcher's semaphore after
@@ -401,7 +403,7 @@ - (void)fetcherDidCreateSession:(GTMSessionFetcher *)fetcher {
       delegateDispatcher.session = fetcherSession;
 
       // Allow other fetchers to request this session now.
-      dispatch_semaphore_signal(delegateDispatcher.sessionCreationSemaphore);
+      dispatch_semaphore_signal(_sessionCreationSemaphore);
     }
   }
 }
@@ -663,17 +665,51 @@ - (void)setReuseSession:(BOOL)shouldReuse {
 }
 
 - (void)resetSession {
+  GTMSessionCheckNotSynchronized(self);
+  dispatch_semaphore_wait(_sessionCreationSemaphore, DISPATCH_TIME_FOREVER);
+
   @synchronized(self) {
     GTMSessionMonitorSynchronized(self);
+    [self resetSessionInternal];
+  }
 
-    // The old dispatchers may be retained as delegates of any ongoing sessions by those sessions.
-    if (_delegateDispatcher) {
-      [self abandonDispatcher];
-      _delegateDispatcher =
-          [[GTMSessionFetcherSessionDelegateDispatcher alloc] initWithParentService:self
-                                                             sessionDiscardInterval:_unusedSessionTimeout];
+  dispatch_semaphore_signal(_sessionCreationSemaphore);
+}
+
+- (void)resetSessionInternal {
+  GTMSessionCheckSynchronized(self);
+
+  // The old dispatchers may be retained as delegates of any ongoing sessions by those sessions.
+  if (_delegateDispatcher) {
+    [self abandonDispatcher];
+    _delegateDispatcher =
+        [[GTMSessionFetcherSessionDelegateDispatcher alloc] initWithParentService:self
+                                                           sessionDiscardInterval:_unusedSessionTimeout];
+  }
+}
+
+- (void)resetSessionForDispatcherDiscardTimer:(NSTimer *)timer {
+  GTMSessionCheckNotSynchronized(self);
+
+  dispatch_semaphore_wait(_sessionCreationSemaphore, DISPATCH_TIME_FOREVER);
+  @synchronized(self) {
+    GTMSessionMonitorSynchronized(self);
+
+    if (_delegateDispatcher.discardTimer == timer) {
+      // If the delegate dispatcher's current discardTimer is the same object as the timer
+      // that fired, no fetcher has recently attempted to start using the session by calling
+      // startSessionUsage, which invalidates and nils out the timer.
+      [self resetSessionInternal];
+    } else {
+      // A fetcher has invalidated the timer between its triggering and now, potentially
+      // meaning a fetcher has requested access to the NSURLSession, and may be in the process
+      // of starting a new task. The dispatcher should not be abandoned, as this can lead
+      // to a race condition between calling -finishTasksAndInvalidate on the NSURLSession
+      // and the fetcher attempting to create a new task.
     }
   }
+
+  dispatch_semaphore_signal(_sessionCreationSemaphore);
 }
 
 - (NSTimeInterval)unusedSessionTimeout {
@@ -695,6 +731,7 @@ - (void)setUnusedSessionTimeout:(NSTimeInterval)timeout {
 
 // This method should be called inside of @synchronized(self)
 - (void)abandonDispatcher {
+  GTMSessionCheckSynchronized(self);
   [_delegateDispatcher abandon];
 }
 
@@ -910,7 +947,6 @@ - (void)setCookieStorageMethod:(NSInteger)cookieStorageMethod {
 @implementation GTMSessionFetcherSessionDelegateDispatcher {
   __weak GTMSessionFetcherService *_parentService;
   NSURLSession *_session;
-  dispatch_semaphore_t _sessionCreationSemaphore;
 
   // The task map maps NSURLSessionTasks to GTMSessionFetchers
   NSMutableDictionary *_taskToFetcherMap;
@@ -920,8 +956,7 @@ @implementation GTMSessionFetcherSessionDelegateDispatcher {
 }
 
 @synthesize discardInterval = _discardInterval,
-            session = _session,
-            sessionCreationSemaphore = _sessionCreationSemaphore;
+            session = _session;
 
 - (instancetype)init {
   [self doesNotRecognizeSelector:_cmd];
@@ -934,7 +969,6 @@ - (instancetype)initWithParentService:(GTMSessionFetcherService *)parentService
   if (self) {
     _discardInterval = discardInterval;
     _parentService = parentService;
-    _sessionCreationSemaphore = dispatch_semaphore_create(1);
   }
   return self;
 }
@@ -946,8 +980,16 @@ - (NSString *)description {
           _taskToFetcherMap.count > 0 ? _taskToFetcherMap : @"<no tasks>"];
 }
 
+- (NSTimer *)discardTimer {
+  GTMSessionCheckNotSynchronized(self);
+  @synchronized(self) {
+    return _discardTimer;
+  }
+}
+
 // This method should be called inside of a @synchronized(self) block.
 - (void)startDiscardTimer {
+  GTMSessionCheckSynchronized(self);
   [_discardTimer invalidate];
   _discardTimer = nil;
   if (_discardInterval > 0) {
@@ -963,6 +1005,7 @@ - (void)startDiscardTimer {
 
 // This method should be called inside of a @synchronized(self) block.
 - (void)destroyDiscardTimer {
+  GTMSessionCheckSynchronized(self);
   [_discardTimer invalidate];
   _discardTimer = nil;
 }
@@ -977,24 +1020,23 @@ - (void)discardTimerFired:(NSTimer *)timer {
       service = _parentService;
     }
   }
-  // Ask the service to abandon us. It will create a new delegate dispatcher
-  // which will have a distinct session.
-  //
-  // Since finishing this session takes a while, it's better for the service to have a new
-  // delegate dispatcher while the tasks on this session's delegate dispatcher finish up.
+
+  // Inform the service that the discard timer has fired, and should check whether the
+  // service can abandon us. -resetSession cannot be called directly, as there is a
+  // race condition that must be guarded against with the NSURLSession being returned
+  // from sessionForFetcherCreation outside other locks. The service can take steps
+  // to prevent resetting the session if that has occurred.
   //
-  // We want to call the service from outside of a @synchronized section.
-  [service resetSession];
+  // The service must be called from outside the @synchronized block.
+  [service resetSessionForDispatcherDiscardTimer:timer];
 }
 
 - (void)abandon {
-  dispatch_semaphore_wait(_sessionCreationSemaphore, DISPATCH_TIME_FOREVER);
   @synchronized(self) {
     GTMSessionMonitorSynchronized(self);
 
     [self destroySessionAndTimer];
   }
-  dispatch_semaphore_signal(_sessionCreationSemaphore);
 }
 
 - (void)startSessionUsage {
@@ -1007,6 +1049,7 @@ - (void)startSessionUsage {
 
 // This method should be called inside of a @synchronized(self) block.
 - (void)destroySessionAndTimer {
+  GTMSessionCheckSynchronized(self);
   [self destroyDiscardTimer];
 
   // Break any retain cycle from the session holding the delegate.