Mark ASRunLoopQueue as drained if it contains only NULLs (#558)

* Mark ASRunLoopQueue as drained if it contains only NULLs * Update CHANGELOG.md * Cover ASRunLoopQueue with tests * Include PR link in CHANGELOG.md * Replace license header of ASRunLoopQueueTests.m with correct one * Insert a nil in _internalQueue to ensure compaction, instead of maintaining a state for _isQueueDrained
2026-01-06 13:12:49 +00:00 · 2017-09-11 22:32:25 +02:00
parent 7c7a4acf0e
commit 002c2d6978
5 changed files with 184 additions and 3 deletions
--- a/AsyncDisplayKit.xcodeproj/project.pbxproj
+++ b/AsyncDisplayKit.xcodeproj/project.pbxproj
@@ -96,6 +96,7 @@
 		3917EBD41E9C2FC400D04A01 /* _ASCollectionReusableView.h in Headers */ = {isa = PBXBuildFile; fileRef = 3917EBD21E9C2FC400D04A01 /* _ASCollectionReusableView.h */; settings = {ATTRIBUTES = (Private, ); }; };
 		3917EBD51E9C2FC400D04A01 /* _ASCollectionReusableView.m in Sources */ = {isa = PBXBuildFile; fileRef = 3917EBD31E9C2FC400D04A01 /* _ASCollectionReusableView.m */; };
 		3C9C128519E616EF00E942A0 /* ASTableViewTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 3C9C128419E616EF00E942A0 /* ASTableViewTests.mm */; };
+		4E9127691F64157600499623 /* ASRunLoopQueueTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 4E9127681F64157600499623 /* ASRunLoopQueueTests.m */; };
 		509E68601B3AED8E009B9150 /* ASScrollDirection.m in Sources */ = {isa = PBXBuildFile; fileRef = 205F0E111B371BD7007741D0 /* ASScrollDirection.m */; };
 		509E68611B3AEDA0009B9150 /* ASAbstractLayoutController.h in Headers */ = {isa = PBXBuildFile; fileRef = 205F0E171B37339C007741D0 /* ASAbstractLayoutController.h */; };
 		509E68621B3AEDA5009B9150 /* ASAbstractLayoutController.mm in Sources */ = {isa = PBXBuildFile; fileRef = 205F0E181B37339C007741D0 /* ASAbstractLayoutController.mm */; };
@@ -624,6 +625,7 @@
 		4640521B1A3F83C40061C0BA /* ASTableLayoutController.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ASTableLayoutController.h; sourceTree = "<group>"; };
 		4640521C1A3F83C40061C0BA /* ASTableLayoutController.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = ASTableLayoutController.m; sourceTree = "<group>"; };
 		4640521D1A3F83C40061C0BA /* ASLayoutController.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ASLayoutController.h; sourceTree = "<group>"; };
+		4E9127681F64157600499623 /* ASRunLoopQueueTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = ASRunLoopQueueTests.m; sourceTree = "<group>"; };
 		68355B2E1CB5799E001D4E68 /* ASImageNode+AnimatedImage.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = "ASImageNode+AnimatedImage.mm"; sourceTree = "<group>"; };
 		68355B361CB57A5A001D4E68 /* ASPINRemoteImageDownloader.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = ASPINRemoteImageDownloader.m; sourceTree = "<group>"; };
 		68355B371CB57A5A001D4E68 /* ASImageContainerProtocolCategories.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ASImageContainerProtocolCategories.h; sourceTree = "<group>"; };
@@ -1219,6 +1221,7 @@
 				69FEE53C1D95A9AF0086F066 /* ASLayoutElementStyleTests.m */,
 				695BE2541DC1245C008E6EA5 /* ASWrapperSpecSnapshotTests.mm */,
 				699B83501E3C1BA500433FA4 /* ASLayoutSpecTests.m */,
+				4E9127681F64157600499623 /* ASRunLoopQueueTests.m */,
 			);
 			path = Tests;
 			sourceTree = "<group>";
@@ -2140,6 +2143,7 @@
 				696FCB311D6E46050093471E /* ASBackgroundLayoutSpecSnapshotTests.mm in Sources */,
 				CC583AD81EF9BDC300134156 /* OCMockObject+ASAdditions.m in Sources */,
 				69FEE53D1D95A9AF0086F066 /* ASLayoutElementStyleTests.m in Sources */,
+				4E9127691F64157600499623 /* ASRunLoopQueueTests.m in Sources */,
 				CC4981B31D1A02BE004E13CC /* ASTableViewThrashTests.m in Sources */,
 				CC54A81E1D7008B300296A24 /* ASDispatchTests.m in Sources */,
 				CCE4F9B31F0D60AC00062E4E /* ASIntegerMapTests.m in Sources */,
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -21,6 +21,7 @@
 - [Breaking] Rename ASCollectionGalleryLayoutSizeProviding to ASCollectionGalleryLayoutPropertiesProviding. Besides a fixed item size, it now can provide interitem and line spacings, as well as section inset [Huy Nguyen](https://github.com/nguyenhuy) [#496](https://github.com/TextureGroup/Texture/pull/496) [#533](https://github.com/TextureGroup/Texture/pull/533)
 - Deprecate `-[ASDisplayNode displayWillStart]` in favor of `-displayWillStartAsynchronously:` [Huy Nguyen](https://github.com/nguyenhuy) [536](https://github.com/TextureGroup/Texture/pull/536)
 - Add support for URLs on ASNetworkImageNode. [Garrett Moon](https://github.com/garrettmoon)
+- Mark ASRunLoopQueue as drained if it contains only NULLs [Cesar Estebanez](https://github.com/cesteban) [#558](https://github.com/TextureGroup/Texture/pull/558)

 ##2.4
 - Fix an issue where inserting/deleting sections could lead to inconsistent supplementary element behavior. [Adlai Holler](https://github.com/Adlai-Holler)
--- a/Source/ASRunLoopQueue.h
+++ b/Source/ASRunLoopQueue.h
@@ -41,6 +41,8 @@ AS_SUBCLASSING_RESTRICTED

 - (void)enqueue:(ObjectType)object;

+@property (nonatomic, readonly) BOOL isEmpty;
+
@property (nonatomic, assign) NSUInteger batchSize;           // Default == 1.
@property (nonatomic, assign) BOOL ensureExclusiveMembership; // Default == YES.  Set-like behavior.

--- a/Source/ASRunLoopQueue.mm
+++ b/Source/ASRunLoopQueue.mm
@@ -348,12 +348,12 @@ typedef enum {
  {
    ASDN::MutexLocker l(_internalQueueLock);

-    // Early-exit if the queue is empty.
    NSInteger internalQueueCount = _internalQueue.count;
+    // Early-exit if the queue is empty.
    if (internalQueueCount == 0) {
      return;
    }
-    
+
    ASSignpostStart(ASSignpostRunLoopQueueBatch);

    // Snatch the next batch of items.
@@ -382,6 +382,14 @@ typedef enum {
      }
    }

+    if (foundItemCount == 0) {
+      // If _internalQueue holds weak references, and all of them just become NULL, then the array
+      // is never marked as needsCompletion, and compact will return early, not removing the NULL's.
+      // Inserting a NULL here ensures the compaction will take place.
+      // See http://www.openradar.me/15396578 and https://stackoverflow.com/a/40274426/1136669
+      [_internalQueue addPointer:NULL];
+    }
+
    [_internalQueue compact];
    if (_internalQueue.count == 0) {
      isQueueDrained = YES;
@@ -434,10 +442,16 @@ typedef enum {

  if (!foundObject) {
    [_internalQueue addPointer:(__bridge void *)object];
-    
+
    CFRunLoopSourceSignal(_runLoopSource);
    CFRunLoopWakeUp(_runLoop);
  }
 }

+- (BOOL)isEmpty
+{
+  ASDN::MutexLocker l(_internalQueueLock);
+  return _internalQueue.count == 0;
+}
+
@end
--- a/Tests/ASRunLoopQueueTests.m
+++ b/Tests/ASRunLoopQueueTests.m
@@ -0,0 +1,160 @@
+//
+//  ASRunLoopQueueTests.m
+//  Texture
+//
+//  Copyright (c) 2017-present, Pinterest, Inc.  All rights reserved.
+//  Licensed under the Apache License, Version 2.0 (the "License");
+//  you may not use this file except in compliance with the License.
+//  You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+
+#import <XCTest/XCTest.h>
+#import <AsyncDisplayKit/ASRunLoopQueue.h>
+
+static NSTimeInterval const kRunLoopRunTime = 0.001; // Allow the RunLoop to run for one millisecond each time.
+
+@interface ASRunLoopQueueTests : XCTestCase
+
+@end
+
+@implementation ASRunLoopQueueTests
+
+#pragma mark enqueue tests
+
+- (void)testEnqueueNilObjectsToQueue
+{
+  ASRunLoopQueue *queue = [[ASRunLoopQueue alloc] initWithRunLoop:CFRunLoopGetMain() retainObjects:YES handler:nil];
+  id object = nil;
+  [queue enqueue:object];
+  XCTAssertTrue(queue.isEmpty);
+}
+
+- (void)testEnqueueSameObjectTwiceToDefaultQueue
+{
+  id object = [[NSObject alloc] init];
+  __unsafe_unretained id weakObject = object;
+  __block NSUInteger dequeuedCount = 0;
+  ASRunLoopQueue *queue = [[ASRunLoopQueue alloc] initWithRunLoop:CFRunLoopGetMain() retainObjects:YES handler:^(id  _Nonnull dequeuedItem, BOOL isQueueDrained) {
+    if (dequeuedItem == weakObject) {
+      dequeuedCount++;
+    }
+  }];
+  [queue enqueue:object];
+  [queue enqueue:object];
+  [[NSRunLoop mainRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:kRunLoopRunTime]];
+  XCTAssert(dequeuedCount == 1);
+}
+
+- (void)testEnqueueSameObjectTwiceToNonExclusiveMembershipQueue
+{
+  id object = [[NSObject alloc] init];
+  __unsafe_unretained id weakObject = object;
+  __block NSUInteger dequeuedCount = 0;
+  ASRunLoopQueue *queue = [[ASRunLoopQueue alloc] initWithRunLoop:CFRunLoopGetMain() retainObjects:YES handler:^(id  _Nonnull dequeuedItem, BOOL isQueueDrained) {
+    if (dequeuedItem == weakObject) {
+      dequeuedCount++;
+    }
+  }];
+  queue.ensureExclusiveMembership = NO;
+  [queue enqueue:object];
+  [queue enqueue:object];
+  [[NSRunLoop mainRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:kRunLoopRunTime]];
+  XCTAssert(dequeuedCount == 2);
+}
+
+#pragma mark processQueue tests
+
+- (void)testDefaultQueueProcessObjectsOneAtATime
+{
+  ASRunLoopQueue *queue = [[ASRunLoopQueue alloc] initWithRunLoop:CFRunLoopGetMain() retainObjects:YES handler:^(id  _Nonnull dequeuedItem, BOOL isQueueDrained) {
+    [NSThread sleepForTimeInterval:kRunLoopRunTime * 2]; // So each element takes more time than the available
+  }];
+  [queue enqueue:[[NSObject alloc] init]];
+  [queue enqueue:[[NSObject alloc] init]];
+  [[NSRunLoop mainRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:kRunLoopRunTime]];
+  XCTAssertFalse(queue.isEmpty);
+}
+
+- (void)testQueueProcessObjectsInBatchesOfSpecifiedSize
+{
+  ASRunLoopQueue *queue = [[ASRunLoopQueue alloc] initWithRunLoop:CFRunLoopGetMain() retainObjects:YES handler:^(id  _Nonnull dequeuedItem, BOOL isQueueDrained) {
+    [NSThread sleepForTimeInterval:kRunLoopRunTime * 2]; // So each element takes more time than the available
+  }];
+  queue.batchSize = 2;
+  [queue enqueue:[[NSObject alloc] init]];
+  [queue enqueue:[[NSObject alloc] init]];
+  [[NSRunLoop mainRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:kRunLoopRunTime]];
+  XCTAssertTrue(queue.isEmpty);
+}
+
+- (void)testQueueOnlySendsIsDrainedForLastObjectInBatch
+{
+  id objectA = [[NSObject alloc] init];
+  id objectB = [[NSObject alloc] init];
+  __unsafe_unretained id weakObjectA = objectA;
+  __unsafe_unretained id weakObjectB = objectB;
+  __block BOOL isQueueDrainedWhenProcessingA = NO;
+  __block BOOL isQueueDrainedWhenProcessingB = NO;
+  ASRunLoopQueue *queue = [[ASRunLoopQueue alloc] initWithRunLoop:CFRunLoopGetMain() retainObjects:YES handler:^(id  _Nonnull dequeuedItem, BOOL isQueueDrained) {
+    if (dequeuedItem == weakObjectA) {
+      isQueueDrainedWhenProcessingA = isQueueDrained;
+    } else if (dequeuedItem == weakObjectB) {
+      isQueueDrainedWhenProcessingB = isQueueDrained;
+    }
+  }];
+  queue.batchSize = 2;
+  [queue enqueue:objectA];
+  [queue enqueue:objectB];
+  [[NSRunLoop mainRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:kRunLoopRunTime]];
+  XCTAssertFalse(isQueueDrainedWhenProcessingA);
+  XCTAssertTrue(isQueueDrainedWhenProcessingB);
+}
+
+#pragma mark strong/weak tests
+
+- (void)testStrongQueueRetainsObjects
+{
+  id object = [[NSObject alloc] init];
+  __unsafe_unretained id weakObject = object;
+  __block BOOL didProcessObject = NO;
+  ASRunLoopQueue *queue = [[ASRunLoopQueue alloc] initWithRunLoop:CFRunLoopGetMain() retainObjects:YES handler:^(id  _Nonnull dequeuedItem, BOOL isQueueDrained) {
+    if (dequeuedItem == weakObject) {
+      didProcessObject = YES;
+    }
+  }];
+  [queue enqueue:object];
+  object = nil;
+  [[NSRunLoop mainRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:kRunLoopRunTime]];
+  XCTAssertTrue(didProcessObject);
+}
+
+- (void)testWeakQueueDoesNotRetainsObjects
+{
+  id object = [[NSObject alloc] init];
+  __unsafe_unretained id weakObject = object;
+  __block BOOL didProcessObject = NO;
+  ASRunLoopQueue *queue = [[ASRunLoopQueue alloc] initWithRunLoop:CFRunLoopGetMain() retainObjects:NO handler:^(id  _Nonnull dequeuedItem, BOOL isQueueDrained) {
+    if (dequeuedItem == weakObject) {
+      didProcessObject = YES;
+    }
+  }];
+  [queue enqueue:object];
+  object = nil;
+  [[NSRunLoop mainRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:kRunLoopRunTime]];
+  XCTAssertFalse(didProcessObject);
+}
+
+- (void)testWeakQueueWithAllDeallocatedObjectsIsDrained
+{
+  ASRunLoopQueue *queue = [[ASRunLoopQueue alloc] initWithRunLoop:CFRunLoopGetMain() retainObjects:NO handler:nil];
+  id object = [[NSObject alloc] init];
+  [queue enqueue:object];
+  object = nil;
+  XCTAssertFalse(queue.isEmpty);
+  [[NSRunLoop mainRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:kRunLoopRunTime]];
+  XCTAssertTrue(queue.isEmpty);
+}
+
+@end