From 1f042e960dc9047be120d79a504a5e7c3b5840f4 Mon Sep 17 00:00:00 2001 From: Michael Schneider Date: Tue, 28 Mar 2017 02:42:59 -0700 Subject: [PATCH] [ASNetworkImageNode] Improve locking (#3187) * Locking improvements for ASNetworkImageNode * Fix typo * Address comments * Address comments * Change comment to kick build --- AsyncDisplayKit.xcodeproj/project.pbxproj | 4 + Source/ASImageNode+AnimatedImage.mm | 130 ++++-- Source/ASImageNode.mm | 31 +- Source/ASNetworkImageNode.mm | 419 ++++++++++-------- .../ASImageNode+AnimatedImagePrivate.h | 6 + Source/Private/ASImageNode+Private.h | 16 + 6 files changed, 382 insertions(+), 224 deletions(-) create mode 100644 Source/Private/ASImageNode+Private.h diff --git a/AsyncDisplayKit.xcodeproj/project.pbxproj b/AsyncDisplayKit.xcodeproj/project.pbxproj index 1e07392f23..0338da70c1 100644 --- a/AsyncDisplayKit.xcodeproj/project.pbxproj +++ b/AsyncDisplayKit.xcodeproj/project.pbxproj @@ -122,6 +122,7 @@ 68FC85E61CE29B9400EDD713 /* ASNavigationController.m in Sources */ = {isa = PBXBuildFile; fileRef = 68FC85DD1CE29AB700EDD713 /* ASNavigationController.m */; }; 68FC85EA1CE29C7D00EDD713 /* ASVisibilityProtocols.h in Headers */ = {isa = PBXBuildFile; fileRef = 68FC85E71CE29C7D00EDD713 /* ASVisibilityProtocols.h */; settings = {ATTRIBUTES = (Public, ); }; }; 68FC85EC1CE29C7D00EDD713 /* ASVisibilityProtocols.m in Sources */ = {isa = PBXBuildFile; fileRef = 68FC85E81CE29C7D00EDD713 /* ASVisibilityProtocols.m */; }; + 6900C5F41E8072DA00BCD75C /* ASImageNode+Private.h in Headers */ = {isa = PBXBuildFile; fileRef = 6900C5F31E8072DA00BCD75C /* ASImageNode+Private.h */; }; 6907C2581DC4ECFE00374C66 /* ASObjectDescriptionHelpers.h in Headers */ = {isa = PBXBuildFile; fileRef = 6907C2561DC4ECFE00374C66 /* ASObjectDescriptionHelpers.h */; settings = {ATTRIBUTES = (Public, ); }; }; 6907C25A1DC4ECFE00374C66 /* ASObjectDescriptionHelpers.m in Sources */ = {isa = PBXBuildFile; fileRef = 6907C2571DC4ECFE00374C66 /* ASObjectDescriptionHelpers.m */; }; 690C35621E055C5D00069B91 /* ASDimensionInternal.mm in Sources */ = {isa = PBXBuildFile; fileRef = 690C35601E055C5D00069B91 /* ASDimensionInternal.mm */; }; @@ -558,6 +559,7 @@ 68FC85E11CE29B7E00EDD713 /* ASTabBarController.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = ASTabBarController.m; sourceTree = ""; }; 68FC85E71CE29C7D00EDD713 /* ASVisibilityProtocols.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ASVisibilityProtocols.h; sourceTree = ""; }; 68FC85E81CE29C7D00EDD713 /* ASVisibilityProtocols.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = ASVisibilityProtocols.m; sourceTree = ""; }; + 6900C5F31E8072DA00BCD75C /* ASImageNode+Private.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = "ASImageNode+Private.h"; sourceTree = ""; }; 6907C2561DC4ECFE00374C66 /* ASObjectDescriptionHelpers.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ASObjectDescriptionHelpers.h; sourceTree = ""; }; 6907C2571DC4ECFE00374C66 /* ASObjectDescriptionHelpers.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = ASObjectDescriptionHelpers.m; sourceTree = ""; }; 690C35601E055C5D00069B91 /* ASDimensionInternal.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = ASDimensionInternal.mm; sourceTree = ""; }; @@ -1188,6 +1190,7 @@ 058D0A0C195D050800B7D73C /* ASDisplayNodeInternal.h */, 6959433D1D70815300B0EE1F /* ASDisplayNodeLayout.h */, 6959433C1D70815300B0EE1F /* ASDisplayNodeLayout.mm */, + 6900C5F31E8072DA00BCD75C /* ASImageNode+Private.h */, 68B8A4DB1CBD911D007E4543 /* ASImageNode+AnimatedImagePrivate.h */, 058D0A0D195D050800B7D73C /* ASImageNode+CGExtras.h */, 058D0A0E195D050800B7D73C /* ASImageNode+CGExtras.m */, @@ -1541,6 +1544,7 @@ B35062261B010EFD0018CF92 /* ASRangeController.h in Headers */, 34EFC76E1B701CF400AD841F /* ASRatioLayoutSpec.h in Headers */, DB55C2671C641AE4004EDCF5 /* ASContextTransitioning.h in Headers */, + 6900C5F41E8072DA00BCD75C /* ASImageNode+Private.h in Headers */, 68B0277B1C1A79D60041016B /* ASDisplayNode+Beta.h in Headers */, B350622D1B010EFD0018CF92 /* ASScrollDirection.h in Headers */, 254C6B751BF94DF4003EC431 /* ASTextKitComponents.h in Headers */, diff --git a/Source/ASImageNode+AnimatedImage.mm b/Source/ASImageNode+AnimatedImage.mm index f390097bc8..14f7f4f155 100644 --- a/Source/ASImageNode+AnimatedImage.mm +++ b/Source/ASImageNode+AnimatedImage.mm @@ -17,12 +17,18 @@ #import #import #import +#import #import #import #import #import #import + +@interface ASNetworkImageNode (Private) +- (void)_locked_setDefaultImage:(UIImage *)image; +@end + NSString *const ASAnimatedImageDefaultRunLoopMode = NSRunLoopCommonModes; @implementation ASImageNode (AnimatedImage) @@ -32,6 +38,11 @@ NSString *const ASAnimatedImageDefaultRunLoopMode = NSRunLoopCommonModes; - (void)setAnimatedImage:(id )animatedImage { ASDN::MutexLocker l(_animatedImageLock); + [self _locked_setAnimatedImage:animatedImage]; +} + +- (void)_locked_setAnimatedImage:(id )animatedImage +{ if (ASObjectIsEqual(_animatedImage, animatedImage)) { return; } @@ -42,17 +53,19 @@ NSString *const ASAnimatedImageDefaultRunLoopMode = NSRunLoopCommonModes; __weak ASImageNode *weakSelf = self; if ([animatedImage respondsToSelector:@selector(setCoverImageReadyCallback:)]) { animatedImage.coverImageReadyCallback = ^(UIImage *coverImage) { - [weakSelf coverImageCompleted:coverImage]; + // In this case the lock is already gone we have to call the unlocked version therefore + [weakSelf setCoverImageCompleted:coverImage]; }; } if (animatedImage.playbackReady) { - [self animatedImageFileReady]; + [self _locked_setShouldAnimate:YES]; + } else { + animatedImage.playbackReadyCallback = ^{ + // In this case the lock is already gone we have to call the unlocked version therefore + [self setShouldAnimate:YES]; + }; } - - animatedImage.playbackReadyCallback = ^{ - [weakSelf animatedImageFileReady]; - }; } } @@ -65,14 +78,10 @@ NSString *const ASAnimatedImageDefaultRunLoopMode = NSRunLoopCommonModes; - (void)setAnimatedImagePaused:(BOOL)animatedImagePaused { ASDN::MutexLocker l(_animatedImageLock); + _animatedImagePaused = animatedImagePaused; - ASPerformBlockOnMainThread(^{ - if (animatedImagePaused) { - [self stopAnimating]; - } else { - [self startAnimating]; - } - }); + + [self _locked_setShouldAnimate:!animatedImagePaused]; } - (BOOL)animatedImagePaused @@ -81,29 +90,37 @@ NSString *const ASAnimatedImageDefaultRunLoopMode = NSRunLoopCommonModes; return _animatedImagePaused; } -- (void)coverImageCompleted:(UIImage *)coverImage +- (void)setCoverImageCompleted:(UIImage *)coverImage { - BOOL setCoverImage = YES; - { - ASDN::MutexLocker l(_displayLinkLock); - if (_displayLink != nil && _displayLink.paused == NO) { - setCoverImage = NO; - } - } + ASDN::MutexLocker l(_animatedImageLock); + [self _locked_setCoverImageCompleted:coverImage]; +} + +- (void)_locked_setCoverImageCompleted:(UIImage *)coverImage +{ + _displayLinkLock.lock(); + BOOL setCoverImage = (_displayLink == nil) || _displayLink.paused; + _displayLinkLock.unlock(); if (setCoverImage) { - [self setCoverImage:coverImage]; + [self _locked_setCoverImage:coverImage]; } } - (void)setCoverImage:(UIImage *)coverImage +{ + ASDN::MutexLocker l(_animatedImageLock); + [self _locked_setCoverImage:coverImage]; +} + +- (void)_locked_setCoverImage:(UIImage *)coverImage { //If we're a network image node, we want to set the default image so //that it will correctly be restored if it exits the range. if ([self isKindOfClass:[ASNetworkImageNode class]]) { - [(ASNetworkImageNode *)self setDefaultImage:coverImage]; + [(ASNetworkImageNode *)self _locked_setDefaultImage:coverImage]; } else { - self.image = coverImage; + [self _locked_setImage:coverImage]; } } @@ -128,31 +145,64 @@ NSString *const ASAnimatedImageDefaultRunLoopMode = NSRunLoopCommonModes; _animatedImageRunLoopMode = runLoopMode; } -- (void)animatedImageFileReady +- (void)setShouldAnimate:(BOOL)shouldAnimate { - ASPerformBlockOnMainThread(^{ - [self startAnimating]; - }); + ASDN::MutexLocker l(_animatedImageLock); + [self _locked_setShouldAnimate:shouldAnimate]; } +- (void)_locked_setShouldAnimate:(BOOL)shouldAnimate +{ + // This test is explicitly done and not ASPerformBlockOnMainThread as this would perform the block immediately + // on main if called on main thread and we have to call methods locked or unlocked based on which thread we are on + if (ASDisplayNodeThreadIsMain()) { + if (shouldAnimate) { + [self _locked_startAnimating]; + } else { + [self _locked_stopAnimating]; + } + } else { + // We have to dispatch to the main thread and call the regular methods as the lock is already gone if the + // block is called + dispatch_async(dispatch_get_main_queue(), ^{ + if (shouldAnimate) { + [self startAnimating]; + } else { + [self stopAnimating]; + } + }); + } +} + +#pragma mark - Animating + - (void)startAnimating { ASDisplayNodeAssertMainThread(); - if (ASInterfaceStateIncludesVisible(self.interfaceState) == NO) { + + ASDN::MutexLocker l(_animatedImageLock); + [self _locked_startAnimating]; +} + +- (void)_locked_startAnimating +{ + // It should be safe to call self.interfaceState in this case as it will only grab the lock of the superclass + if (!ASInterfaceStateIncludesVisible(self.interfaceState)) { return; } - if (self.animatedImagePaused) { + if (_animatedImagePaused) { return; } - if (self.animatedImage.playbackReady == NO) { + if (_animatedImage.playbackReady == NO) { return; } #if ASAnimatedImageDebug NSLog(@"starting animation: %p", self); #endif + ASDN::MutexLocker l(_displayLinkLock); if (_displayLink == nil) { _playHead = 0; @@ -167,6 +217,16 @@ NSString *const ASAnimatedImageDefaultRunLoopMode = NSRunLoopCommonModes; - (void)stopAnimating { + ASDisplayNodeAssertMainThread(); + + ASDN::MutexLocker l(_animatedImageLock); + [self _locked_stopAnimating]; +} + +- (void)_locked_stopAnimating +{ + ASDisplayNodeAssertMainThread(); + #if ASAnimatedImageDebug NSLog(@"stopping animation: %p", self); #endif @@ -175,9 +235,11 @@ NSString *const ASAnimatedImageDefaultRunLoopMode = NSRunLoopCommonModes; _displayLink.paused = YES; self.lastDisplayLinkFire = 0; - [self.animatedImage clearAnimatedImageCache]; + [_animatedImage clearAnimatedImageCache]; } +#pragma mark - ASDisplayNode + - (void)didEnterVisibleState { ASDisplayNodeAssertMainThread(); @@ -197,6 +259,8 @@ NSString *const ASAnimatedImageDefaultRunLoopMode = NSRunLoopCommonModes; [self stopAnimating]; } +#pragma mark - Display Link Callbacks + - (void)displayLinkFired:(CADisplayLink *)displayLink { ASDisplayNodeAssertMainThread(); @@ -251,6 +315,8 @@ NSString *const ASAnimatedImageDefaultRunLoopMode = NSRunLoopCommonModes; @end +#pragma mark - ASImageNode(AnimatedImageInvalidation) + @implementation ASImageNode(AnimatedImageInvalidation) - (void)invalidateAnimatedImage diff --git a/Source/ASImageNode.mm b/Source/ASImageNode.mm index 33af93dc04..1591d8187f 100644 --- a/Source/ASImageNode.mm +++ b/Source/ASImageNode.mm @@ -223,21 +223,25 @@ struct ASImageNodeDrawParameters { - (void)setImage:(UIImage *)image { - { - ASDN::MutexLocker l(__instanceLock__); - if (ASObjectIsEqual(_image, image)) { - return; - } + ASDN::MutexLocker l(__instanceLock__); + [self _locked_setImage:image]; +} - _image = image; +- (void)_locked_setImage:(UIImage *)image +{ + if (ASObjectIsEqual(_image, image)) { + return; } - [self setNeedsLayout]; - - if (image) { + _image = image; + + if (image != nil) { + + // We explicitly call setNeedsDisplay in this case, although we know setNeedsDisplay will be called with lock held. + // Therefore we have to be careful in methods that are involved with setNeedsDisplay to not run into a deadlock [self setNeedsDisplay]; - // Debugging + // For debugging purposes we don't care about locking for now if ([ASImageNode shouldShowImageScalingOverlay] && _debugLabelNode == nil) { ASPerformBlockOnMainThread(^{ _debugLabelNode = [[ASTextNode alloc] init]; @@ -245,7 +249,7 @@ struct ASImageNodeDrawParameters { [self addSubnode:_debugLabelNode]; }); } - + } else { self.contents = nil; } @@ -257,6 +261,11 @@ struct ASImageNodeDrawParameters { return _image; } +- (UIImage *)_locked_Image +{ + return _image; +} + - (void)setPlaceholderColor:(UIColor *)placeholderColor { _placeholderColor = placeholderColor; diff --git a/Source/ASNetworkImageNode.mm b/Source/ASNetworkImageNode.mm index 1e3e734826..35d660976a 100755 --- a/Source/ASNetworkImageNode.mm +++ b/Source/ASNetworkImageNode.mm @@ -15,6 +15,8 @@ #import #import #import +#import +#import #import #if PIN_REMOTE_IMAGE @@ -49,16 +51,17 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; unsigned int delegateDidLoadImage:1; } _delegateFlags; - //set on init only - __weak id _downloader; - __weak id _cache; + // Immutable and set on init only. We don't need to lock in this case. + __weak id _downloader; struct { unsigned int downloaderImplementsSetProgress:1; unsigned int downloaderImplementsSetPriority:1; unsigned int downloaderImplementsAnimatedImage:1; } _downloaderFlags; + // Immutable and set on init only. We don't need to lock in this case. + __weak id _cache; struct { unsigned int cacheSupportsClearing:1; unsigned int cacheSupportsSynchronousFetch:1; @@ -113,24 +116,34 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; - (void)setImage:(UIImage *)image { ASDN::MutexLocker l(__instanceLock__); - + [self _locked_setImage:image]; +} + +- (void)_locked_setImage:(UIImage *)image +{ BOOL imageWasSetExternally = (image != nil); - BOOL shouldCancelAndClear = imageWasSetExternally && imageWasSetExternally != _imageWasSetExternally; + BOOL shouldCancelAndClear = imageWasSetExternally && (imageWasSetExternally != _imageWasSetExternally); _imageWasSetExternally = imageWasSetExternally; if (shouldCancelAndClear) { ASDisplayNodeAssertNil(_URL, @"Directly setting an image on an ASNetworkImageNode causes it to behave like an ASImageNode instead of an ASNetworkImageNode. If this is what you want, set the URL to nil first."); - [self _cancelDownloadAndClearImage]; _URL = nil; + [self _locked_cancelDownloadAndClearImage]; } - [self _setImage:image]; + [self _locked__setImage:image]; } +/// Setter for private image property. See @c setImage why this is needed - (void)_setImage:(UIImage *)image { super.image = image; } +- (void)_locked__setImage:(UIImage *)image +{ + [super _locked_setImage:image]; +} + - (void)setURL:(NSURL *)URL { [self setURL:URL resetToDefault:YES]; @@ -149,20 +162,16 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; return; } - [self _cancelImageDownload]; + [self _locked_cancelImageDownload]; + _imageLoaded = NO; _URL = URL; - BOOL hasURL = _URL == nil; + BOOL hasURL = (_URL == nil); if (reset || hasURL) { - [self _setImage:_defaultImage]; - /* We want to maintain the order that currentImageQuality is set regardless of the calling thread, - so always use a dispatch_async to ensure that we queue the operations in the correct order. - (see comment in displayDidFinish) */ - dispatch_async(dispatch_get_main_queue(), ^{ - self.currentImageQuality = hasURL ? 0.0 : 1.0; - }); + [self _locked_setCurrentImageQuality:(hasURL ? 0.0 : 1.0)]; + [self _locked__setImage:_defaultImage]; } } @@ -179,20 +188,21 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; { ASDN::MutexLocker l(__instanceLock__); + [self _locked_setDefaultImage:defaultImage]; +} + +- (void)_locked_setDefaultImage:(UIImage *)defaultImage +{ if (ASObjectIsEqual(defaultImage, _defaultImage)) { return; } + _defaultImage = defaultImage; if (!_imageLoaded) { - BOOL hasURL = _URL == nil; - /* We want to maintain the order that currentImageQuality is set regardless of the calling thread, - so always use a dispatch_async to ensure that we queue the operations in the correct order. - (see comment in displayDidFinish) */ - dispatch_async(dispatch_get_main_queue(), ^{ - self.currentImageQuality = hasURL ? 0.0 : 1.0; - }); - [self _setImage:defaultImage]; + [self _locked_setCurrentImageQuality:((_URL == nil) ? 0.0 : 1.0)]; + [self _locked__setImage:defaultImage]; + } } @@ -214,6 +224,29 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; return _currentImageQuality; } +/** + * Always use this methods internally to update the current image quality + * We want to maintain the order that currentImageQuality is set regardless of the calling thread, + * so we always have to dispatch to the main threadto ensure that we queue the operations in the correct order. + * (see comment in displayDidFinish) + */ +- (void)_setCurrentImageQuality:(CGFloat)imageQuality +{ + ASDN::MutexLocker l(__instanceLock__); + [self _locked_setCurrentImageQuality:imageQuality]; +} + +- (void)_locked_setCurrentImageQuality:(CGFloat)imageQuality +{ + dispatch_async(dispatch_get_main_queue(), ^{ + // As the setting of the image quality is dispatched the lock is gone by the time the block is executing. + // Therefore we have to grab the lock again + __instanceLock__.lock(); + _currentImageQuality = imageQuality; + __instanceLock__.unlock(); + }); +} + - (void)setRenderedImageQuality:(CGFloat)renderedImageQuality { ASDN::MutexLocker l(__instanceLock__); @@ -276,14 +309,14 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; if (asynchronously == NO && _cacheFlags.cacheSupportsSynchronousFetch) { ASDN::MutexLocker l(__instanceLock__); + if (_imageLoaded == NO && _URL && _downloadIdentifier == nil) { UIImage *result = [[_cache synchronouslyFetchedCachedImageWithURL:_URL] asdk_image]; if (result) { - [self _setImage:result]; + [self _locked_setCurrentImageQuality:1.0]; + [self _locked__setImage:result]; + _imageLoaded = YES; - dispatch_async(dispatch_get_main_queue(), ^{ - _currentImageQuality = 1.0; - }); } } } @@ -292,9 +325,11 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; [self didEnterPreloadState]; if (self.image == nil && _downloaderFlags.downloaderImplementsSetPriority) { - ASDN::MutexLocker l(__instanceLock__); - if (_downloadIdentifier != nil) { - [_downloader setPriority:ASImageDownloaderPriorityImminent withDownloadIdentifier:_downloadIdentifier]; + __instanceLock__.lock(); + id downloadIdentifier = _downloadIdentifier; + __instanceLock__.unlock(); + if (downloadIdentifier != nil) { + [_downloader setPriority:ASImageDownloaderPriorityImminent withDownloadIdentifier:downloadIdentifier]; } } } @@ -305,15 +340,12 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; { [super didEnterVisibleState]; - id downloadIdentifier = nil; - - { - ASDN::MutexLocker l(__instanceLock__); - + __instanceLock__.lock(); + id downloadIdentifier = nil; if (_downloaderFlags.downloaderImplementsSetPriority) { downloadIdentifier = _downloadIdentifier; } - } + __instanceLock__.unlock(); if (downloadIdentifier != nil) { [_downloader setPriority:ASImageDownloaderPriorityVisible withDownloadIdentifier:downloadIdentifier]; @@ -325,15 +357,13 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; - (void)didExitVisibleState { [super didExitVisibleState]; - - id downloadIdentifier = nil; - { - ASDN::MutexLocker l(__instanceLock__); + __instanceLock__.lock(); + id downloadIdentifier = nil; if (_downloaderFlags.downloaderImplementsSetPriority) { downloadIdentifier = _downloadIdentifier; } - } + __instanceLock__.unlock(); if (downloadIdentifier != nil) { [_downloader setPriority:ASImageDownloaderPriorityPreload withDownloadIdentifier:downloadIdentifier]; @@ -346,45 +376,41 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; { [super didExitPreloadState]; - { - ASDN::MutexLocker l(__instanceLock__); - // If the image was set explicitly we don't want to remove it while exiting the preload state - if (_imageWasSetExternally) { - return; - } - - [self _cancelDownloadAndClearImage]; + __instanceLock__.lock(); + BOOL imageWasSetExternally = _imageWasSetExternally; + __instanceLock__.unlock(); + // If the image was set explicitly we don't want to remove it while exiting the preload state + if (imageWasSetExternally) { + return; } + + [self _cancelDownloadAndClearImage]; } - (void)didEnterPreloadState { [super didEnterPreloadState]; - { - ASDN::MutexLocker l(__instanceLock__); - // Image was set externally no need to load an image - [self _lazilyLoadImageIfNecessary]; - } + // Image was set externally no need to load an image + [self _lazilyLoadImageIfNecessary]; } -#pragma mark - Private methods, safe to call without lock +#pragma mark - Progress - (void)handleProgressImage:(UIImage *)progressImage progress:(CGFloat)progress downloadIdentifier:(nullable id)downloadIdentifier { - ASDN::MutexLocker l(__instanceLock__); + __instanceLock__.lock(); + // Getting a result back for a different download identifier, download must not have been successfully canceled if (ASObjectIsEqual(_downloadIdentifier, downloadIdentifier) == NO && downloadIdentifier != nil) { return; } - [self _setImage:progressImage]; - dispatch_async(dispatch_get_main_queue(), ^{ - // See comment in -displayDidFinish for why this must be dispatched to main - self.currentImageQuality = progress; - }); -} + + [self _locked_setCurrentImageQuality:progress]; + [self _locked__setImage:progressImage]; -#pragma mark - Private methods, call with lock held + __instanceLock__.unlock(); +} - (void)_updateProgressImageBlockOnDownloaderIfNeeded { @@ -394,16 +420,12 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; } // Read state. - BOOL shouldRender; - id oldDownloadIDForProgressBlock; - id newDownloadIDForProgressBlock; - BOOL clearAndReattempt = NO; - { - ASDN::MutexLocker l(__instanceLock__); - shouldRender = _shouldRenderProgressImages && ASInterfaceStateIncludesVisible(_interfaceState); - oldDownloadIDForProgressBlock = _downloadIdentifierForProgressBlock; - newDownloadIDForProgressBlock = shouldRender ? _downloadIdentifier : nil; - } + __instanceLock__.lock(); + BOOL shouldRender = _shouldRenderProgressImages && ASInterfaceStateIncludesVisible(_interfaceState); + id oldDownloadIDForProgressBlock = _downloadIdentifierForProgressBlock; + id newDownloadIDForProgressBlock = shouldRender ? _downloadIdentifier : nil; + BOOL clearAndReattempt = NO; + __instanceLock__.unlock(); // If we're already bound to the correct download, we're done. if (ASObjectIsEqual(oldDownloadIDForProgressBlock, newDownloadIDForProgressBlock)) { @@ -448,14 +470,46 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; - (void)_cancelDownloadAndClearImage { - [self _cancelImageDownload]; - [self _clearImage]; - if (_cacheFlags.cacheSupportsClearing && _URL != nil) { - [_cache clearFetchedImageFromCacheWithURL:_URL]; + ASDN::MutexLocker l(__instanceLock__); + [self _locked_cancelDownloadAndClearImage]; +} + +- (void)_locked_cancelDownloadAndClearImage +{ + [self _locked_cancelImageDownload]; + + // Destruction of bigger images on the main thread can be expensive + // and can take some time, so we dispatch onto a bg queue to + // actually dealloc. + UIImage *image = [self _locked_Image]; + UIImage *defaultImage = _defaultImage; + CGSize imageSize = image.size; + BOOL shouldReleaseImageOnBackgroundThread = imageSize.width > kMinReleaseImageOnBackgroundSize.width || + imageSize.height > kMinReleaseImageOnBackgroundSize.height; + if (shouldReleaseImageOnBackgroundThread) { + ASPerformBackgroundDeallocation(image); + } + + [self _locked_setAnimatedImage:nil]; + [self _locked_setCurrentImageQuality:0.0]; + [self _locked__setImage:defaultImage]; + + _imageLoaded = NO; + + if (_cacheFlags.cacheSupportsClearing) { + if (_URL != nil) { + [_cache clearFetchedImageFromCacheWithURL:_URL]; + } } } - (void)_cancelImageDownload +{ + ASDN::MutexLocker l(__instanceLock__); + [self _locked_cancelImageDownload]; +} + +- (void)_locked_cancelImageDownload { if (!_downloadIdentifier) { return; @@ -469,27 +523,6 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; _cacheUUID = nil; } -- (void)_clearImage -{ - // Destruction of bigger images on the main thread can be expensive - // and can take some time, so we dispatch onto a bg queue to - // actually dealloc. - UIImage *image = self.image; - CGSize imageSize = image.size; - BOOL shouldReleaseImageOnBackgroundThread = imageSize.width > kMinReleaseImageOnBackgroundSize.width || - imageSize.height > kMinReleaseImageOnBackgroundSize.height; - if (shouldReleaseImageOnBackgroundThread) { - ASPerformBackgroundDeallocation(image); - } - self.animatedImage = nil; - [self _setImage:_defaultImage]; - _imageLoaded = NO; - // See comment in -displayDidFinish for why this must be dispatched to main - dispatch_async(dispatch_get_main_queue(), ^{ - self.currentImageQuality = 0.0; - }); -} - - (void)_downloadImageWithCompletion:(void (^)(id imageContainer, NSError*, id downloadIdentifier))finished { ASPerformBlockOnBackgroundThread(^{ @@ -540,75 +573,82 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; - (void)_lazilyLoadImageIfNecessary { - // FIXME: We should revisit locking in this method (e.g. to access the instance variables at the top, and holding lock while calling delegate) - if (!_imageLoaded && _URL != nil && _downloadIdentifier == nil) { - { - ASDN::MutexLocker l(__instanceLock__); - if (_delegateFlags.delegateDidStartFetchingData) { - [_delegate imageNodeDidStartFetchingData:self]; - } + __instanceLock__.lock(); + __weak id delegate = _delegate; + BOOL delegateDidStartFetchingData = _delegateFlags.delegateDidStartFetchingData; + BOOL isImageLoaded = _imageLoaded; + NSURL *URL = _URL; + id currentDownloadIdentifier = _downloadIdentifier; + __instanceLock__.unlock(); + + if (!isImageLoaded && URL != nil && currentDownloadIdentifier == nil) { + if (delegateDidStartFetchingData) { + [delegate imageNodeDidStartFetchingData:self]; } - if (_URL.isFileURL) { - { + if (URL.isFileURL) { + dispatch_async(dispatch_get_main_queue(), ^{ ASDN::MutexLocker l(__instanceLock__); + + // Bail out if not the same URL anymore + if (!ASObjectIsEqual(URL, _URL)) { + return; + } + + if (_shouldCacheImage) { + [self _locked__setImage:[UIImage imageNamed:_URL.path.lastPathComponent]]; + } else { + // First try to load the path directly, for efficiency assuming a developer who + // doesn't want caching is trying to be as minimal as possible. + UIImage *nonAnimatedImage = [UIImage imageWithContentsOfFile:_URL.path]; + if (nonAnimatedImage == nil) { + // If we couldn't find it, execute an -imageNamed:-like search so we can find resources even if the + // extension is not provided in the path. This allows the same path to work regardless of shouldCacheImage. + NSString *filename = [[NSBundle mainBundle] pathForResource:_URL.path.lastPathComponent ofType:nil]; + if (filename != nil) { + nonAnimatedImage = [UIImage imageWithContentsOfFile:filename]; + } + } - dispatch_async(dispatch_get_main_queue(), ^{ - if (self.shouldCacheImage) { - [self _setImage:[UIImage imageNamed:_URL.path.lastPathComponent]]; + // If the file may be an animated gif and then created an animated image. + id animatedImage = nil; + if (_downloaderFlags.downloaderImplementsAnimatedImage) { + NSData *data = [NSData dataWithContentsOfURL:_URL]; + if (data != nil) { + animatedImage = [_downloader animatedImageWithData:data]; + + if ([animatedImage respondsToSelector:@selector(isDataSupported:)] && [animatedImage isDataSupported:data] == NO) { + animatedImage = nil; + } + } + } + + if (animatedImage != nil) { + [self _locked_setAnimatedImage:animatedImage]; } else { - // First try to load the path directly, for efficiency assuming a developer who - // doesn't want caching is trying to be as minimal as possible. - UIImage *nonAnimatedImage = [UIImage imageWithContentsOfFile:_URL.path]; - if (nonAnimatedImage == nil) { - // If we couldn't find it, execute an -imageNamed:-like search so we can find resources even if the - // extension is not provided in the path. This allows the same path to work regardless of shouldCacheImage. - NSString *filename = [[NSBundle mainBundle] pathForResource:_URL.path.lastPathComponent ofType:nil]; - if (filename != nil) { - nonAnimatedImage = [UIImage imageWithContentsOfFile:filename]; - } - } - - // If the file may be an animated gif and then created an animated image. - id animatedImage = nil; - if (_downloaderFlags.downloaderImplementsAnimatedImage) { - NSData *data = [NSData dataWithContentsOfURL:_URL]; - if (data != nil) { - animatedImage = [_downloader animatedImageWithData:data]; - - if ([animatedImage respondsToSelector:@selector(isDataSupported:)] && [animatedImage isDataSupported:data] == NO) { - animatedImage = nil; - } - } - } - - if (animatedImage != nil) { - self.animatedImage = animatedImage; - } else { - [self _setImage:nonAnimatedImage]; - } + [self _locked__setImage:nonAnimatedImage]; } + } - _imageLoaded = YES; - /* We want to maintain the order that currentImageQuality is set regardless of the calling thread, - so always use a dispatch_async to ensure that we queue the operations in the correct order. - (see comment in displayDidFinish) */ - dispatch_async(dispatch_get_main_queue(), ^{ - self.currentImageQuality = 1.0; - }); - if (_delegateFlags.delegateDidLoadImage) { - [_delegate imageNode:self didLoadImage:self.image]; - } - }); - } + _imageLoaded = YES; + + [self _locked_setCurrentImageQuality:1.0]; + + if (_delegateFlags.delegateDidLoadImage) { + ASDN::MutexUnlocker u(__instanceLock__); + [delegate imageNode:self didLoadImage:self.image]; + } + }); } else { __weak __typeof__(self) weakSelf = self; void (^finished)(id , NSError *, id downloadIdentifier) = ^(id imageContainer, NSError *error, id downloadIdentifier) { + __typeof__(self) strongSelf = weakSelf; if (strongSelf == nil) { return; } + // Grab the lock for the rest of the block ASDN::MutexLocker l(strongSelf->__instanceLock__); //Getting a result back for a different download identifier, download must not have been successfully canceled @@ -617,43 +657,50 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; } if (imageContainer != nil) { - strongSelf->_imageLoaded = YES; - if ([imageContainer asdk_animatedImageData] && _downloaderFlags.downloaderImplementsAnimatedImage) { - strongSelf.animatedImage = [_downloader animatedImageWithData:[imageContainer asdk_animatedImageData]]; + [strongSelf _locked_setCurrentImageQuality:1.0]; + if ([imageContainer asdk_animatedImageData] && strongSelf->_downloaderFlags.downloaderImplementsAnimatedImage) { + id animatedImage = [strongSelf->_downloader animatedImageWithData:[imageContainer asdk_animatedImageData]]; + [strongSelf _locked_setAnimatedImage:animatedImage]; } else { - [strongSelf _setImage:[imageContainer asdk_image]]; + [strongSelf _locked__setImage:[imageContainer asdk_image]]; } - dispatch_async(dispatch_get_main_queue(), ^{ - strongSelf->_currentImageQuality = 1.0; - }); + strongSelf->_imageLoaded = YES; } strongSelf->_downloadIdentifier = nil; - strongSelf->_cacheUUID = nil; if (imageContainer != nil) { if (strongSelf->_delegateFlags.delegateDidLoadImage) { - [strongSelf->_delegate imageNode:strongSelf didLoadImage:strongSelf.image]; + ASDN::MutexUnlocker u(strongSelf->__instanceLock__); + [delegate imageNode:strongSelf didLoadImage:strongSelf.image]; } - } - else if (error && strongSelf->_delegateFlags.delegateDidFailWithError) { - [strongSelf->_delegate imageNode:strongSelf didFailWithError:error]; + } else if (error && strongSelf->_delegateFlags.delegateDidFailWithError) { + ASDN::MutexUnlocker u(strongSelf->__instanceLock__); + [delegate imageNode:strongSelf didFailWithError:error]; } }; + // As the _cache and _downloader is only set once in the intializer we don't have to use a + // lock in here if (_cache != nil) { NSUUID *cacheUUID = [NSUUID UUID]; - _cacheUUID = cacheUUID; + __instanceLock__.lock(); + _cacheUUID = cacheUUID; + __instanceLock__.unlock(); - [_cache cachedImageWithURL:_URL + [_cache cachedImageWithURL:URL callbackQueue:dispatch_get_main_queue() completion:^(id imageContainer) { // If the cache UUID changed, that means this request was cancelled. - if (!ASObjectIsEqual(_cacheUUID, cacheUUID)) { + __instanceLock__.lock(); + NSUUID *currentCacheUUID = _cacheUUID; + __instanceLock__.unlock(); + + if (!ASObjectIsEqual(currentCacheUUID, cacheUUID)) { return; } - + if ([imageContainer asdk_image] == nil && _downloader != nil) { [self _downloadImageWithCompletion:finished]; } else { @@ -672,19 +719,29 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; - (void)displayDidFinish { [super displayDidFinish]; + + id delegate = nil; + + __instanceLock__.lock(); + if (_delegateFlags.delegateDidFinishDecoding && self.layer.contents != nil) { + /* We store the image quality in _currentImageQuality whenever _image is set. On the following displayDidFinish, we'll know that + _currentImageQuality is the quality of the image that has just finished rendering. In order for this to be accurate, we + need to be sure we are on main thread when we set _currentImageQuality. Otherwise, it is possible for _currentImageQuality + to be modified at a point where it is too late to cancel the main thread's previous display (the final sentinel check has passed), + but before the displayDidFinish of the previous display pass is called. In this situation, displayDidFinish would be called and we + would set _renderedImageQuality to the new _currentImageQuality, but the actual quality of the rendered image should be the previous + value stored in _currentImageQuality. */ - ASDN::MutexLocker l(__instanceLock__); - if (_delegateFlags.delegateDidFinishDecoding && self.layer.contents != nil) { - /* We store the image quality in _currentImageQuality whenever _image is set. On the following displayDidFinish, we'll know that - _currentImageQuality is the quality of the image that has just finished rendering. In order for this to be accurate, we - need to be sure we are on main thread when we set _currentImageQuality. Otherwise, it is possible for _currentImageQuality - to be modified at a point where it is too late to cancel the main thread's previous display (the final sentinel check has passed), - but before the displayDidFinish of the previous display pass is called. In this situation, displayDidFinish would be called and we - would set _renderedImageQuality to the new _currentImageQuality, but the actual quality of the rendered image should be the previous - value stored in _currentImageQuality. */ - - _renderedImageQuality = _currentImageQuality; - [self.delegate imageNodeDidFinishDecoding:self]; + _renderedImageQuality = _currentImageQuality; + + // Assign the delegate to be used + delegate = _delegate; + } + + __instanceLock__.unlock(); + + if (delegate != nil) { + [delegate imageNodeDidFinishDecoding:self]; } } diff --git a/Source/Private/ASImageNode+AnimatedImagePrivate.h b/Source/Private/ASImageNode+AnimatedImagePrivate.h index 364ce7cf8e..bdd0a15e49 100644 --- a/Source/Private/ASImageNode+AnimatedImagePrivate.h +++ b/Source/Private/ASImageNode+AnimatedImagePrivate.h @@ -32,6 +32,12 @@ extern NSString *const ASAnimatedImageDefaultRunLoopMode; @end +@interface ASImageNode (AnimatedImagePrivate) + +- (void)_locked_setAnimatedImage:(id )animatedImage; + +@end + @interface ASImageNode (AnimatedImageInvalidation) diff --git a/Source/Private/ASImageNode+Private.h b/Source/Private/ASImageNode+Private.h new file mode 100644 index 0000000000..e0880ba5a7 --- /dev/null +++ b/Source/Private/ASImageNode+Private.h @@ -0,0 +1,16 @@ +// +// ASImageNode+Private.h +// AsyncDisplayKit +// +// Created by Michael Schneider on 3/20/17. +// Copyright © 2017 Facebook. All rights reserved. +// + +#pragma once + +@interface ASImageNode (Private) + +- (void)_locked_setImage:(UIImage *)image; +- (UIImage *)_locked_Image; + +@end