Skip to content

Commit

Permalink
Mark ASRunLoopQueue as drained if it contains only NULLs (#558)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
cestebanez authored and garrettmoon committed Sep 11, 2017
1 parent 7c7a4ac commit 002c2d6
Show file tree
Hide file tree
Showing 5 changed files with 184 additions and 3 deletions.
4 changes: 4 additions & 0 deletions AsyncDisplayKit.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -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 */; };
Expand Down Expand Up @@ -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>"; };
Expand Down Expand Up @@ -1219,6 +1221,7 @@
69FEE53C1D95A9AF0086F066 /* ASLayoutElementStyleTests.m */,
695BE2541DC1245C008E6EA5 /* ASWrapperSpecSnapshotTests.mm */,
699B83501E3C1BA500433FA4 /* ASLayoutSpecTests.m */,
4E9127681F64157600499623 /* ASRunLoopQueueTests.m */,
);
path = Tests;
sourceTree = "<group>";
Expand Down Expand Up @@ -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 */,
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 2 additions & 0 deletions Source/ASRunLoopQueue.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
20 changes: 17 additions & 3 deletions Source/ASRunLoopQueue.mm
Original file line number Diff line number Diff line change
Expand Up @@ -348,12 +348,12 @@ - (void)processQueue
{
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.
Expand Down Expand Up @@ -382,6 +382,14 @@ - (void)processQueue
}
}

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;
Expand Down Expand Up @@ -434,10 +442,16 @@ - (void)enqueue:(id)object

if (!foundObject) {
[_internalQueue addPointer:(__bridge void *)object];

CFRunLoopSourceSignal(_runLoopSource);
CFRunLoopWakeUp(_runLoop);
}
}

- (BOOL)isEmpty
{
ASDN::MutexLocker l(_internalQueueLock);
return _internalQueue.count == 0;
}

@end
160 changes: 160 additions & 0 deletions Tests/ASRunLoopQueueTests.m
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 002c2d6

Please sign in to comment.