Stricter locking assertions #1024

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

Source/ASDisplayNode+Layout.mm

+
+    {
+      ASDN::MutexUnlocker u(__instanceLock__);
+      [self _u_setNeedsLayoutFromAbove];


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

Source/ASDisplayNode+Layout.mm

  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]) {
+  if ([self _locked_isLayoutTransitionInvalid]) {


While having the lock, call the _locked_ method.

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

Source/ASDisplayNode+Layout.mm


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


Cache these properties to improve readability of the following lines

#notAPerfOptimization

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

Source/ASDisplayNode+Layout.mm

-  }
-
-  [self transitionLayoutWithSizeRange:sizeRange
+  [self transitionLayoutWithSizeRange:[self _constrainedSizeForLayoutPass]


Easier to just call the "unlocked" variant.

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

Source/ASDisplayNode.mm

@@ -584,14 +590,20 @@ - (BOOL)isNodeLoaded
  if (ASDisplayNodeThreadIsMain()) {
    // Because the view and layer can only be created and destroyed on Main, that is also the only thread
    // where the state of this property can change. As an optimization, we can avoid locking.
-    return [self _locked_isNodeLoaded];
+    return [self _isNodeLoaded];


Can't call the _locked_ method here

Good catch!

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

Source/ASVideoNode.mm

@@ -360,6 +361,8 @@ - (void)observeValueForKeyPath:(NSString *)keyPath ofObject:(id)object change:(N
        if (self.playerState == ASVideoNodePlayerStateLoading && _delegateFlags.delegateVideoNodeDidRecoverFromStall) {
          [self.delegate videoNodeDidRecoverFromStall:self];
        }
+
+        ASUnlockScope(self);


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

Source/ASDisplayNode+Layout.mm

  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

Source/ASDisplayNode.mm

+  return (_view != nil || (_layer != nil && _flags.layerBacked));
+}
+
+- (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

Source/ASDisplayNode.mm

+}
+
+/// 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

Source/ASDisplayNode.mm

 {
-  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

nguyenhuy requested review from appleguy, maicki, Adlai-Holler and garrettmoon 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stricter locking assertions #1024

Stricter locking assertions #1024

nguyenhuy commented Jul 12, 2018 •

edited

Loading

nguyenhuy commented Jul 12, 2018

nguyenhuy Jul 12, 2018

nguyenhuy Jul 12, 2018

nguyenhuy Jul 12, 2018

nguyenhuy Jul 12, 2018

nguyenhuy Jul 12, 2018

garrettmoon Jul 12, 2018

nguyenhuy Jul 12, 2018

garrettmoon Jul 12, 2018

ghost commented Jul 12, 2018

Adlai-Holler left a comment

nguyenhuy commented Jul 12, 2018

garrettmoon Jul 12, 2018

garrettmoon Jul 12, 2018

nguyenhuy Jul 12, 2018

Adlai-Holler Jul 12, 2018

nguyenhuy Jul 13, 2018

Adlai-Holler Jul 13, 2018

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

Stricter locking assertions #1024

Stricter locking assertions #1024

Conversation

nguyenhuy commented Jul 12, 2018 • edited Loading

nguyenhuy commented Jul 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jul 12, 2018

Adlai-Holler left a comment

Choose a reason for hiding this comment

nguyenhuy commented Jul 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyenhuy commented Jul 12, 2018 •

edited

Loading