[Minor Breaking API] Make deallocation queues more reliable (TextureG…

…roup#651) * Make our async deallocation functions take a double pointer, so we can be sure we've released before the queue drains * Make it a class property * Fix the return type * Use a locker * Improve release notes
AtomTickets · Apr 25, 2018 · 735802d · 735802d
1 parent 7f980e3
commit 735802d
Show file tree

Hide file tree

Showing 13 changed files with 44 additions and 24 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,7 @@
 - [ASNetworkImageNode] New delegate callback to tell the consumer whether the image was loaded from cache or download. [Adlai Holler](https://github.com/Adlai-Holler)
 - [Layout] Fixes a deadlock in layout. [#638](https://github.com/TextureGroup/Texture/pull/638) [Garrett Moon](https://github.com/garrettmoon)
 - Updated to be backwards compatible with Xcode 8. [Adlai Holler](https://github.com/Adlai-Holler)
+- [API CHANGES] `ASPerformMainThreadDeallocation` and `ASPerformBackgroundDeallocation` functions take `id *` instead of `id` and they're now more reliable. Also, in Swift, `ASDeallocQueue.sharedDeallocationQueue() -> ASDeallocQueue.sharedDeallocationQueue`. [Adlai Holler](https://github.com/Adlai-Holler) [#651](https://github.com/TextureGroup/Texture/pull/651)
 
 ## 2.6
 - [Xcode 9] Updated to require Xcode 9 (to fix warnings) [Garrett Moon](https://github.com/garrettmoon)

diff --git a/Source/ASCollectionView.mm b/Source/ASCollectionView.mm
@@ -332,8 +332,8 @@ - (void)dealloc
   [self setAsyncDataSource:nil];
 
   // Data controller & range controller may own a ton of nodes, let's deallocate those off-main.
-  ASPerformBackgroundDeallocation(_dataController);
-  ASPerformBackgroundDeallocation(_rangeController);
+  ASPerformBackgroundDeallocation(&_dataController);
+  ASPerformBackgroundDeallocation(&_rangeController);
 }
 
 #pragma mark -

diff --git a/Source/ASDisplayNode.mm b/Source/ASDisplayNode.mm
@@ -440,7 +440,7 @@ - (void)_scheduleIvarsForMainDeallocation
       // is still deallocated on a background thread
       object_setIvar(self, ivar, nil);
 
-      ASPerformMainThreadDeallocation(value);
+      ASPerformMainThreadDeallocation(&value);
     } else {
       as_log_debug(ASMainThreadDeallocationLog(), "%@: Not trampolining ivar '%s' value %@.", self, ivar_getName(ivar), value);
     }

diff --git a/Source/ASDisplayNodeExtras.h b/Source/ASDisplayNodeExtras.h
@@ -35,7 +35,7 @@
 #endif
 
 /// For deallocation of objects on the main thread across multiple run loops.
-extern void ASPerformMainThreadDeallocation(_Nullable id object);
+extern void ASPerformMainThreadDeallocation(id _Nullable __strong * _Nonnull objectPtr);
 
 // Because inline methods can't be extern'd and need to be part of the translation unit of code
 // that compiles with them to actually inline, we both declare and define these in the header.

diff --git a/Source/ASDisplayNodeExtras.mm b/Source/ASDisplayNodeExtras.mm
@@ -23,8 +23,7 @@
 #import <queue>
 #import <AsyncDisplayKit/ASRunLoopQueue.h>
 
-extern void ASPerformMainThreadDeallocation(_Nullable id object)
-{
+extern void ASPerformMainThreadDeallocation(id _Nullable __strong * _Nonnull objectPtr) {
   /**
    * UIKit components must be deallocated on the main thread. We use this shared
    * run loop queue to gradually deallocate them across many turns of the main run loop.
@@ -35,8 +34,14 @@ extern void ASPerformMainThreadDeallocation(_Nullable id object)
     queue = [[ASRunLoopQueue alloc] initWithRunLoop:CFRunLoopGetMain() retainObjects:YES handler:nil];
     queue.batchSize = 10;
   });
-  if (object != nil) {
-  	[queue enqueue:object];
+
+  if (objectPtr != NULL && *objectPtr != nil) {
+    // Lock queue while enqueuing and releasing, so that there's no risk
+    // that the queue will release before we get a chance to release.
+    [queue lock];
+    [queue enqueue:*objectPtr];   // Retain, +1
+    *objectPtr = nil;             // Release, +0
+    [queue unlock];               // (After queue drains), release, -1
   }
 }
 

diff --git a/Source/ASImageNode.mm b/Source/ASImageNode.mm
@@ -278,7 +278,7 @@ - (void)_locked_setImage:(UIImage *)image
   BOOL shouldReleaseImageOnBackgroundThread = oldImageSize.width > kMinReleaseImageOnBackgroundSize.width
                                               || oldImageSize.height > kMinReleaseImageOnBackgroundSize.height;
   if (shouldReleaseImageOnBackgroundThread) {
-    ASPerformBackgroundDeallocation(oldImage);
+    ASPerformBackgroundDeallocation(&oldImage);
   }
 }
 

diff --git a/Source/ASMultiplexImageNode.mm b/Source/ASMultiplexImageNode.mm
@@ -532,10 +532,10 @@ - (void)_clearImage
   CGSize imageSize = image.size;
   BOOL shouldReleaseImageOnBackgroundThread = imageSize.width > kMinReleaseImageOnBackgroundSize.width ||
   imageSize.height > kMinReleaseImageOnBackgroundSize.height;
+  [self _setImage:nil];
   if (shouldReleaseImageOnBackgroundThread) {
-    ASPerformBackgroundDeallocation(image);
+    ASPerformBackgroundDeallocation(&image);
   }
-  [self _setImage:nil];
 }
 
 #pragma mark -

diff --git a/Source/ASRunLoopQueue.h b/Source/ASRunLoopQueue.h
@@ -21,7 +21,7 @@
 NS_ASSUME_NONNULL_BEGIN
 
 AS_SUBCLASSING_RESTRICTED
-@interface ASRunLoopQueue<ObjectType> : NSObject
+@interface ASRunLoopQueue<ObjectType> : NSObject <NSLocking>
 
 /**
  * Create a new queue with the given run loop and handler.
@@ -51,11 +51,11 @@ AS_SUBCLASSING_RESTRICTED
 AS_SUBCLASSING_RESTRICTED
 @interface ASDeallocQueue : NSObject
 
-+ (instancetype)sharedDeallocationQueue;
+@property (class, atomic, readonly) ASDeallocQueue *sharedDeallocationQueue;
 
 - (void)test_drain;
 
-- (void)releaseObjectInBackground:(id)object;
+- (void)releaseObjectInBackground:(id __strong _Nullable * _Nonnull)objectPtr;
 
 @end
 

diff --git a/Source/ASRunLoopQueue.mm b/Source/ASRunLoopQueue.mm
@@ -45,7 +45,7 @@ @implementation ASDeallocQueue {
   ASDN::RecursiveMutex _queueLock;
 }
 
-+ (instancetype)sharedDeallocationQueue
++ (ASDeallocQueue *)sharedDeallocationQueue
 {
   static ASDeallocQueue *deallocQueue = nil;
   static dispatch_once_t onceToken;
@@ -55,16 +55,18 @@ + (instancetype)sharedDeallocationQueue
   return deallocQueue;
 }
 
-- (void)releaseObjectInBackground:(id)object
+- (void)releaseObjectInBackground:(id  _Nullable __strong *)objectPtr
 {
   // Disable background deallocation on iOS 8 and below to avoid crashes related to UIAXDelegateClearer (#2767).
   if (!AS_AT_LEAST_IOS9) {
     return;
   }
 
-  _queueLock.lock();
-  _queue.push_back(object);
-  _queueLock.unlock();
+  if (objectPtr != NULL && *objectPtr != nil) {
+    ASDN::MutexLocker l(_queueLock);
+    _queue.push_back(*objectPtr);
+    *objectPtr = nil;
+  }
 }
 
 - (void)threadMain
@@ -454,4 +456,16 @@ - (BOOL)isEmpty
   return _internalQueue.count == 0;
 }
 
+#pragma mark - NSLocking
+
+- (void)lock
+{
+  _internalQueueLock.lock();
+}
+
+- (void)unlock
+{
+  _internalQueueLock.unlock();
+}
+
 @end
diff --git a/Source/ASTableView.mm b/Source/ASTableView.mm
@@ -372,8 +372,8 @@ - (void)dealloc
   [self setAsyncDataSource:nil];
 
   // Data controller & range controller may own a ton of nodes, let's deallocate those off-main
-  ASPerformBackgroundDeallocation(_dataController);
-  ASPerformBackgroundDeallocation(_rangeController);
+  ASPerformBackgroundDeallocation(&_dataController);
+  ASPerformBackgroundDeallocation(&_rangeController);
 }
 
 #pragma mark -

diff --git a/Source/ASViewController.mm b/Source/ASViewController.mm
@@ -96,7 +96,7 @@ - (void)_initializeInstance
 
 - (void)dealloc
 {
-  ASPerformBackgroundDeallocation(_node);
+  ASPerformBackgroundDeallocation(&_node);
 }
 
 - (void)loadView

diff --git a/Source/Private/ASInternalHelpers.h b/Source/Private/ASInternalHelpers.h
@@ -38,7 +38,7 @@ void ASPerformBlockOnMainThread(void (^block)(void));
 void ASPerformBlockOnBackgroundThread(void (^block)(void)); // DISPATCH_QUEUE_PRIORITY_DEFAULT
 
 /// For deallocation of objects on a background thread without GCD overhead / thread explosion
-void ASPerformBackgroundDeallocation(id object);
+void ASPerformBackgroundDeallocation(id __strong _Nullable * _Nonnull object);
 
 CGFloat ASScreenScale(void);
 

diff --git a/Source/Private/ASInternalHelpers.m b/Source/Private/ASInternalHelpers.m
@@ -84,7 +84,7 @@ void ASPerformBlockOnBackgroundThread(void (^block)(void))
   }
 }
 
-void ASPerformBackgroundDeallocation(id object)
+void ASPerformBackgroundDeallocation(id __strong _Nullable * _Nonnull object)
 {
   [[ASDeallocQueue sharedDeallocationQueue] releaseObjectInBackground:object];
 }