Stricter locking assertions by nguyenhuy · Pull Request #1024 · TextureGroup/Texture

nguyenhuy · 2018-07-12T04:52:04Z

Rename ASDisplayNodeAssertLockUnownedByCurrentThread to ASAssertUnlocked, and ASDisplayNodeAssertLockOwnedByCurrentThread to ASAssertLocked -> shorter and hopefully easier to distinguish between the two.
Add assertions to _locked_ and _u_ (i.e "unlocked") methods.
Turn CHECK_LOCKING_SAFETY on by default. After Fix misleading/scary stack trace shown when an assertion occurs during node measurement #1022 and Make sure -_completePendingLayoutTransition is called without the node's instance lock #trivial #1023, we're in a good shape to actually enforce locked/unlocked requirements of internal methods. Our test suite passed, and we'll test more at Pinterest after the sync this week.
Fix ASVideoNode to avoid calling play while holding the lock. That method inserts a subnode and must be called lock free.
Other minor changes.

nguyenhuy · 2018-07-12T04:53:37Z

Note to reviewers: please review this after #1022 and #1023.

nguyenhuy · 2018-07-12T04:55:21Z

Apply the same fix in #1022 here just to be safe, i.e if _u_setNeedsLayoutFromAbove causes an assertion, it'll be correctly shown.

nguyenhuy · 2018-07-12T04:56:08Z

While having the lock, call the _locked_ method.

nguyenhuy · 2018-07-12T04:58:46Z

Cache these properties to improve readability of the following lines

#notAPerfOptimization

nguyenhuy · 2018-07-12T05:00:24Z

Easier to just call the "unlocked" variant.

nguyenhuy · 2018-07-12T05:01:47Z

Can't call the _locked_ method here

Good catch!

nguyenhuy · 2018-07-12T05:03:30Z

As mentions in the description, play shouldn't be called with the lock.

Is it worth adding a comment in code?

ghost · 2018-07-12T16:49:19Z

🚫 CI failed with log

Adlai-Holler

If we're renaming the macro, how about ASAssertLocked and ASAssertUnlocked? Over time I'd like to move away from ASDisplayNode as a prefix, since the framework now encompasses so much more.

nguyenhuy · 2018-07-12T18:24:05Z

Uhh, I like that. Will update!

- Rename `ASDisplayNodeAssertLockUnownedByCurrentThread` to `ASDisplayNodeAssertLockNotHeld`, and `ASDisplayNodeAssertLockOwnedByCurrentThread` to `ASDisplayNodeAssertLockHeld` -> shorter and hopefully easier to distinguish between the two. - Add assertions to `_locked_` and `_u_` (i.e "unlocked") methods. - Turn `CHECK_LOCKING_SAFETY` flag on by default. After TextureGroup#1022 and TextureGroup#1023, we're in a good shape to actually enforce locked/unlocked requirements of internal methods. Our test suite passed, and we'll test more at Pinterest after the sync this week. - Fix ASVideoNode to avoid calling `play` while holding the lock. That method inserts a subnode and must be called lock free. - Other minor changes.

garrettmoon · 2018-07-12T21:06:49Z

+  ASAssertLocked(__instanceLock__);
+
  CGSize boundsSizeForLayout = ASCeilSizeValues(self.threadSafeBounds.size);
+  std::shared_ptr<ASDisplayNodeLayout> pendingLayout = _pendingDisplayNodeLayout;


Do you mind adding a comment explaining the shared_ptr's here?

garrettmoon · 2018-07-12T21:08:07Z

+
+- (BOOL)_isNodeLoaded
 {
  return (_view != nil || (_layer != nil && _flags.layerBacked));


Does this need to be updated with a lock?

This method is called unlocked, and the client is responsible for its safety. Renamed to -_unsafe_unlocked_isNodeLoaded to reflect that.

Adlai-Holler · 2018-07-12T21:53:19Z

+}
+
+/// Called without the lock. Client is responsible for locking safety.
+- (BOOL)_unsafe_unlocked_isNodeLoaded


Can we simplify all of this by just using _layer != nil inline when we want a fast loaded check?

great point!

Adlai-Holler · 2018-07-13T01:37:42Z

+ASDISPLAYNODE_INLINE AS_WARN_UNUSED_RESULT BOOL _ASIsNodeLoaded(ASDisplayNode *node)
 {
-  return (_view != nil || (_layer != nil && _flags.layerBacked));
+  return (node->_view != nil || (node->_layer != nil && node->_flags.layerBacked));


What if we get rid of this function and just put _layer != nil inside the places where we need it?

…huy/Texture into HN-Stricter-Locking-Safety-Checks

…layer backed, the layer should always be set if loaded

- Rename `ASDisplayNodeAssertLockUnownedByCurrentThread` to `ASAssertUnlocked`, and `ASDisplayNodeAssertLockOwnedByCurrentThread` to `ASAssertLocked` -> shorter and hopefully easier to distinguish between the two. - Add assertions to `_locked_` and `_u_` (i.e "unlocked") methods. - Turn `CHECK_LOCKING_SAFETY` flag on by default. After TextureGroup#1022 and TextureGroup#1023, we're in a good shape to actually enforce locked/unlocked requirements of internal methods. Our test suite passed, and we'll test more at Pinterest after the sync this week. - Fix ASVideoNode to avoid calling `play` while holding the lock. That method inserts a subnode and must be called lock free. - Simplify `_loaded(node)` to only nil-check `_layer` because regardless of whether the node is view or layer backed, the layer should always be set if loaded. Use it throughout. - Other minor changes.

nguyenhuy commented Jul 12, 2018

View reviewed changes

Comment thread Source/ASDisplayNode+Layout.mm Outdated

nguyenhuy Jul 12, 2018

Copy link
Copy Markdown

Member Author

Choose a reason for hiding this comment

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

While having the lock, call the _locked_ method.

nguyenhuy commented Jul 12, 2018

View reviewed changes

Comment thread Source/ASDisplayNode+Layout.mm Outdated

nguyenhuy Jul 12, 2018

Copy link
Copy Markdown

Member Author

Choose a reason for hiding this comment

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

Easier to just call the "unlocked" variant.

nguyenhuy commented Jul 12, 2018

View reviewed changes

nguyenhuy requested review from Adlai-Holler, appleguy, garrettmoon and maicki July 12, 2018 05:03

Adlai-Holler reviewed Jul 12, 2018

View reviewed changes

TextureGroup deleted a comment Jul 12, 2018

nguyenhuy force-pushed the HN-Stricter-Locking-Safety-Checks branch from 4a0ebd2 to 465be36 Compare July 12, 2018 20:43

nguyenhuy force-pushed the HN-Stricter-Locking-Safety-Checks branch from 465be36 to c037799 Compare July 12, 2018 20:49

Rename to ASAssertLocked and ASAssertUnlocked

c2bdd1a

garrettmoon reviewed Jul 12, 2018

View reviewed changes

nguyenhuy added 2 commits July 12, 2018 14:17

Revert unnecessary change

48b7fa1

Rename -_isNodeLoaded to -_unsafe_unlocked_isNodeLoaded

b454388

Adlai-Holler approved these changes Jul 12, 2018

View reviewed changes

TextureGroup deleted a comment Jul 12, 2018

Use inline method

49c21ae

Adlai-Holler reviewed Jul 13, 2018

View reviewed changes

Merge branch 'master' into HN-Stricter-Locking-Safety-Checks

372ff89

nguyenhuy added 3 commits July 13, 2018 13:00

Move __loaded(node) macro to ASDisplayNodeInternal and use it throughout

0325062

Merge branch 'HN-Stricter-Locking-Safety-Checks' of github.com:nguyen…

4dd528f

…huy/Texture into HN-Stricter-Locking-Safety-Checks

Simpler node loaded check: regardless of whether the node is view or …

f4234df

…layer backed, the layer should always be set if loaded

nguyenhuy merged commit 0dc97fb into TextureGroup:master Jul 13, 2018

nguyenhuy deleted the HN-Stricter-Locking-Safety-Checks branch July 13, 2018 21:58

Adlai-Holler mentioned this pull request Jul 16, 2018

Turn lock-checking on only when assertions are enabled #trivial #1032

Merged

qiaoyan mentioned this pull request Jul 18, 2018

Locker assertion failure around ASVideoPlayerNode init #1040

Closed

Conversation

nguyenhuy commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nguyenhuy commented Jul 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Jul 12, 2018

Uh oh!

Adlai-Holler left a comment

Choose a reason for hiding this comment

Uh oh!

nguyenhuy commented Jul 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nguyenhuy commented Jul 12, 2018 •

edited

Loading