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.

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

[ASDisplayNode] Always return the thread-safe cornerRadius property, even in slow CALayer rounding mode #749

Merged

Conversation

nguyenhuy
Copy link
Member

@nguyenhuy nguyenhuy commented Jan 16, 2018

  • If the node's property was updated on a background thread and main thread has not executed the block that updates cornerRadius of the node's layer, the layer's property is out-of-date and shouldn't be used.
  • After this change, ASDisplayNode's cornerRadius is the only source of truth and users must always use it instead of CALayer's.

Relevant PR: #465.
Recorded bug here - Pay attention to the "Save" button on the top right corner.

}

- (void)setCornerRadius:(CGFloat)newCornerRadius
{
ASDN::MutexLocker l(__instanceLock__);
Copy link
Member Author

Choose a reason for hiding this comment

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

-updateCornerRoundingWithType:cornerRadius: acquires the lock itself (here).

Copy link
Contributor

@maicki maicki left a comment

Choose a reason for hiding this comment

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

LGTM

Looking over this PR and the initial PR I think we could remove the layerCornerRadius getter / setter now and set the value directly in updateCornerRoundingWithType:cornerRadius:.

@nguyenhuy nguyenhuy changed the title Always return the backing thread-safe cornerRadius property of ASDisplayNode, even in slow CALayer rounding type [ASDisplayNode] Always return the thread-safe cornerRadius property, even in slow CALayer rounding mode Jan 16, 2018
@nguyenhuy nguyenhuy force-pushed the HNCornerRadiusRaceCondition branch 2 times, most recently from 84d114a to 5b13c16 Compare January 16, 2018 18:25
…layNode, even in slow CALayer rounding type

- Failing to do so will introduce race conditions in which the property was updated on a background thread but main thread has not executed the block that updates the property of the node's layer. During that window, the layer's property is out-of-date and can't be used.
- After this change, ASDisplayNode's cornerRadius is the only source of truth and users must always use it instead of CALayer's.
@@ -207,7 +202,6 @@ - (ASCornerRoundingType)cornerRoundingType

- (void)setCornerRoundingType:(ASCornerRoundingType)newRoundingType
{
ASDN::MutexLocker l(__instanceLock__);
[self updateCornerRoundingWithType:newRoundingType cornerRadius:_cornerRadius];
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this public? Think we should read self.cornerRadius?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeap, nice catch!

@nguyenhuy nguyenhuy merged commit b4a269a into TextureGroup:master Feb 8, 2018
@nguyenhuy nguyenhuy deleted the HNCornerRadiusRaceCondition branch February 8, 2018 17:08
bernieperez pushed a commit to AtomTickets/Texture that referenced this pull request Apr 25, 2018
…even in slow CALayer rounding mode (TextureGroup#749)

- Failing to do so will introduce race conditions in which the property was updated on a background thread but main thread has not executed the block that updates the property of the node's layer. During that window, the layer's property is out-of-date and can't be used.
- After this change, ASDisplayNode's cornerRadius is the only source of truth and users must always use it instead of CALayer's.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants