-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
Conversation
@@ -368,8 +369,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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, looks right to me. These methods are definitely "unlocked".
We may run into race conditions that are caused by another thread sneaks between the unlock/lock calls and changes the node's states. Fixing it would require a better architecture. Definitely not in the scope of this PR.
Source/ASDisplayNode+Layout.mm
Outdated
@@ -211,7 +211,7 @@ @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 | |||
{ | |||
as_activity_create_for_scope("Set needs layout from above"); | |||
ASDisplayNodeAssertThreadAffinity(self); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Let's add the thread ownership assertion (i.e ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__)
). We may enable it by default in the future.
@@ -287,8 +287,9 @@ - (void)displayNodeDidInvalidateSizeNewSize:(CGSize)size | |||
} | |||
} | |||
|
|||
- (void)_locked_measureNodeWithBoundsIfNecessary:(CGRect)bounds | |||
- (void)_u_measureNodeWithBoundsIfNecessary:(CGRect)bounds | |||
{ |
There was a problem hiding this comment.
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.
50b8f61
to
620f231
Compare
Good callouts, thank you for the review @nguyenhuy :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @nguyenhuy we can run into race conditions / unknown behavior if layout is going on on other threads for the same node, but for the current circumstances this looks ok to me.
…e. (TextureGroup#638) * Fix a layout deadlock caused by holding the lock and going up the tree. * Add CHANGELOG message * Huy's comments
No description provided.