From 002c2d6978efab95207077811fbccd2d9cccda88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9sar=20Est=C3=A9banez=20Tasc=C3=B3n?= Date: Mon, 11 Sep 2017 22:32:25 +0200 Subject: [PATCH] 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 --- AsyncDisplayKit.xcodeproj/project.pbxproj | 4 + CHANGELOG.md | 1 + Source/ASRunLoopQueue.h | 2 + Source/ASRunLoopQueue.mm | 20 ++- Tests/ASRunLoopQueueTests.m | 160 ++++++++++++++++++++++ 5 files changed, 184 insertions(+), 3 deletions(-) create mode 100644 Tests/ASRunLoopQueueTests.m diff --git a/AsyncDisplayKit.xcodeproj/project.pbxproj b/AsyncDisplayKit.xcodeproj/project.pbxproj index 41389f7219..068938a4d2 100644 --- 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 = ""; }; 4640521C1A3F83C40061C0BA /* ASTableLayoutController.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = ASTableLayoutController.m; sourceTree = ""; }; 4640521D1A3F83C40061C0BA /* ASLayoutController.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ASLayoutController.h; sourceTree = ""; }; + 4E9127681F64157600499623 /* ASRunLoopQueueTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = ASRunLoopQueueTests.m; sourceTree = ""; }; 68355B2E1CB5799E001D4E68 /* ASImageNode+AnimatedImage.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = "ASImageNode+AnimatedImage.mm"; sourceTree = ""; }; 68355B361CB57A5A001D4E68 /* ASPINRemoteImageDownloader.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = ASPINRemoteImageDownloader.m; sourceTree = ""; }; 68355B371CB57A5A001D4E68 /* ASImageContainerProtocolCategories.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ASImageContainerProtocolCategories.h; sourceTree = ""; }; @@ -1219,6 +1221,7 @@ 69FEE53C1D95A9AF0086F066 /* ASLayoutElementStyleTests.m */, 695BE2541DC1245C008E6EA5 /* ASWrapperSpecSnapshotTests.mm */, 699B83501E3C1BA500433FA4 /* ASLayoutSpecTests.m */, + 4E9127681F64157600499623 /* ASRunLoopQueueTests.m */, ); path = Tests; sourceTree = ""; @@ -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 */, diff --git a/CHANGELOG.md b/CHANGELOG.md index fda12670ac..16e6571690 100644 --- 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) diff --git a/Source/ASRunLoopQueue.h b/Source/ASRunLoopQueue.h index 7ff563f409..6b08540ccd 100644 --- 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. diff --git a/Source/ASRunLoopQueue.mm b/Source/ASRunLoopQueue.mm index 004d4abb97..1902ad23d0 100644 --- 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 diff --git a/Tests/ASRunLoopQueueTests.m b/Tests/ASRunLoopQueueTests.m new file mode 100644 index 0000000000..ccf15ae7b7 --- /dev/null +++ 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 +#import + +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