Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a layout deadlock caused by holding the lock and going up the tree. #638

Merged
merged 3 commits into from
Oct 31, 2017
+17 −9
Merged

Fix a layout deadlock caused by holding the lock and going up the tree. #638

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5872bca
Fix a layout deadlock caused by holding the lock and going up the tree.
garrettmoon Oct 24, 2017
b0a9423
Add CHANGELOG message
garrettmoon Oct 24, 2017
620f231
Huy's comments
garrettmoon Oct 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
- [Animated Image] Adds support for animated WebP as well as improves GIF handling. [#605](https://github.com/TextureGroup/Texture/pull/605) [Garrett Moon](https://github.com/garrettmoon)
- [ASCollectionView] Check if batch fetching is needed if batch fetching parameter has been changed. [#624](https://github.com/TextureGroup/Texture/pull/624) [Garrett Moon](https://github.com/garrettmoon)
- [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)

## 2.6
- [Xcode 9] Updated to require Xcode 9 (to fix warnings) [Garrett Moon](https://github.com/garrettmoon)
Expand Down
17 changes: 11 additions & 6 deletions Source/ASDisplayNode+Layout.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,8 +211,9 @@ @implementation ASDisplayNode (ASLayoutInternal)
* @discussion The size of a root node is determined by each subnode. Calling invalidateSize will let the root node know
* that the intrinsic size of the receiver node is no longer valid and a resizing of the root node needs to happen.
*/
- (void)_setNeedsLayoutFromAbove
- (void)_u_setNeedsLayoutFromAbove
{
ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock);
as_activity_create_for_scope("Set needs layout from above");
ASDisplayNodeAssertThreadAffinity(self);

Expand All @@ -227,7 +228,7 @@ - (void)_setNeedsLayoutFromAbove

if (supernode) {
// Threading model requires that we unlock before calling a method on our parent.
[supernode _setNeedsLayoutFromAbove];
[supernode _u_setNeedsLayoutFromAbove];
} else {
// Let the root node method know that the size was invalidated
[self _rootNodeDidInvalidateSize];
Expand Down Expand Up @@ -287,8 +288,10 @@ - (void)displayNodeDidInvalidateSizeNewSize:(CGSize)size
}
}

- (void)_locked_measureNodeWithBoundsIfNecessary:(CGRect)bounds
- (void)_u_measureNodeWithBoundsIfNecessary:(CGRect)bounds
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, let's call ASDisplayNodeAssertLockUnownedByCurrentThread() before acquiring the lock.

ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock);
ASDN::MutexLocker l(__instanceLock__);
// Check if we are a subnode in a layout transition.
// In this case no measurement is needed as it's part of the layout transition
if ([self _isLayoutTransitionInvalid]) {
Expand Down Expand Up @@ -368,8 +371,10 @@ - (void)_locked_measureNodeWithBoundsIfNecessary:(CGRect)bounds
// In this case, we need to detect that we've already asked to be resized to match this
// particular ASLayout object, and shouldn't loop asking again unless we have a different ASLayout.
nextLayout->requestedLayoutFromAbove = YES;
[self _setNeedsLayoutFromAbove];
// Update the layout's version here because _setNeedsLayoutFromAbove calls __setNeedsLayout which in turn increases _layoutVersion
__instanceLock__.unlock();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 3 lines are the important bits which fix the deadlock.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@garrettmoon We do use _calculatedDisplayNodeLayout below — it looks like this would make it possible that another thread could change _calculatedDisplayNodeLayout while this is unlocked (possibly by the main thread calling -layout).

I'm wondering if we should actually be returning after this, or something else like forcing a -layoutIfNeeded. It's a bit weird that we proceed to _completePendingLayoutTransition when, in actuality, we are not yet in a valid full-tree layout state until the parent has -layoutSublayers called by CA.

This might be OK because one could argue that the layout is correct for this node and its children, even if not the full tree.

[self _u_setNeedsLayoutFromAbove];
__instanceLock__.lock();
// Update the layout's version here because _u_setNeedsLayoutFromAbove calls __setNeedsLayout which in turn increases _layoutVersion
// Failing to do this will cause the layout to be invalid immediately
nextLayout->version = _layoutVersion;
}
Expand All @@ -389,7 +394,7 @@ - (void)_locked_measureNodeWithBoundsIfNecessary:(CGRect)bounds

- (ASSizeRange)_locked_constrainedSizeForLayoutPass
{
// TODO: The logic in -_setNeedsLayoutFromAbove seems correct and doesn't use this method.
// TODO: The logic in -_u_setNeedsLayoutFromAbove seems correct and doesn't use this method.
// logic seems correct. For what case does -this method need to do the CGSizeEqual checks?
// IF WE CAN REMOVE BOUNDS CHECKS HERE, THEN WE CAN ALSO REMOVE "REQUESTED FROM ABOVE" CHECK

Expand Down
4 changes: 3 additions & 1 deletion Source/ASDisplayNode.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -911,7 +911,9 @@ - (void)__layout

// This method will confirm that the layout is up to date (and update if needed).
// Importantly, it will also APPLY the layout to all of our subnodes if (unless parent is transitioning).
[self _locked_measureNodeWithBoundsIfNecessary:bounds];
__instanceLock__.unlock();
[self _u_measureNodeWithBoundsIfNecessary:bounds];
__instanceLock__.lock();

[self _locked_layoutPlaceholderIfNecessary];
}
Expand Down
4 changes: 2 additions & 2 deletions Source/Private/ASDisplayNode+FrameworkPrivate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ __unused static NSString * _Nonnull NSStringFromASHierarchyState(ASHierarchyStat
* @discussion The size of a root node is determined by each subnode. Calling invalidateSize will let the root node know
* that the intrinsic size of the receiver node is no longer valid and a resizing of the root node needs to happen.
*/
- (void)_setNeedsLayoutFromAbove;
- (void)_u_setNeedsLayoutFromAbove;

/**
* @abstract Subclass hook for nodes that are acting as root nodes. This method is called if one of the subnodes
Expand All @@ -237,7 +237,7 @@ __unused static NSString * _Nonnull NSStringFromASHierarchyState(ASHierarchyStat
* This method will confirm that the layout is up to date (and update if needed).
* Importantly, it will also APPLY the layout to all of our subnodes if (unless parent is transitioning).
*/
- (void)_locked_measureNodeWithBoundsIfNecessary:(CGRect)bounds;
- (void)_u_measureNodeWithBoundsIfNecessary:(CGRect)bounds;

/**
* Layout all of the subnodes based on the sublayouts
Expand Down