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.
These 3 lines are the important bits which fix the deadlock.
There was a problem hiding this comment.
@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.
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.
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.
Same, let's call ASDisplayNodeAssertLockUnownedByCurrentThread() before acquiring the lock.
garrettmoon
force-pushed
the
fixLayoutDeadlock
branch
from
50b8f61
to
620f231
Compare
|
Good callouts, thank you for the review @nguyenhuy :) |
There was a problem hiding this comment.
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.