Improve locking situation in ASVideoPlayerNode #1042

maicki · 2018-07-19T16:24:48Z

In ASVideoPlayerNode we are currently adding and removing subnodes with lock held. That's not allowed.

Fixes #1040

ghost · 2018-07-19T16:27:31Z

🚫 CI failed with log

ghost · 2018-07-19T16:29:20Z

🚫 CI failed with log

maicki · 2018-07-19T16:29:37Z

Source/ASVideoPlayerNode.mm

@@ -644,9 +671,11 @@ - (void)showSpinner
    }];

    _spinnerNode.style.preferredSize = CGSizeMake(44.0, 44.0);
-
-    [self addSubnode:_spinnerNode];


The spinner can be removed and added on different threads we may should grab the _spinnerNode before unlocking and adding it.

maicki · 2018-07-19T16:29:57Z

Source/ASVideoPlayerNode.mm

@@ -658,7 +687,10 @@ - (void)removeSpinner
  if (!_spinnerNode) {
    return;
  }
-  [_spinnerNode removeFromSupernode];
+  {


We may should grab the _spinnerNode here before unlocking

Let's do this at the end so we don't need to re-acquire the lock.

nguyenhuy

One last thing: Let's tighten up the locking requirements by adding ASAssertUnlocked at the beginning of - -createControls and -removeControls, as well as ASAssertLocked at the beginning of _locked_ methods.

nguyenhuy · 2018-07-20T17:37:11Z

Source/ASVideoPlayerNode.mm

@@ -284,15 +282,23 @@ - (void)createControls

    if (_delegateFlags.delegateCustomControls && _delegateFlags.delegateLayoutSpecForControls) {
      NSDictionary *customControls = [_delegate videoPlayerNodeCustomControls:self];


In general we should avoid calling delegates with the lock held. May not worth address in this PR though.

nguyenhuy · 2018-07-20T17:37:54Z

Source/ASVideoPlayerNode.mm

+        for (var subnode : subnodes) {
+          [self addSubnode:subnode];
+        }
+      }


We can avoid a pair of lock and unlock calls if we add subnodes outside of the outmost lock scope.

Talked to @maicki offline. Let's leave this as is since my suggested approach is a bit more complicated and in case of early return(s), the vector will not be used.

nguyenhuy · 2018-07-20T17:40:16Z

Source/ASVideoPlayerNode.mm

  }

-  [self cleanCachedControls];
+  [self __locked_cleanCachedControls];


I think it's safe to do this before calling removeFromSupernode on cached controls? If yes we can avoid the unlock scope.

nguyenhuy · 2018-07-20T17:41:18Z

Source/ASVideoPlayerNode.mm

 }

- (void)cleanCachedControls
+- (void)__locked_cleanCachedControls


Also, should we use 1 underscore at the beginning of this method's name (i.e - (void)_locked_cleanCachedControls)?

nguyenhuy · 2018-07-20T17:43:59Z

Source/ASVideoPlayerNode.mm

@@ -658,7 +687,10 @@ - (void)removeSpinner
  if (!_spinnerNode) {
    return;
  }
-  [_spinnerNode removeFromSupernode];
+  {


Let's do this at the end so we don't need to re-acquire the lock.

nguyenhuy · 2018-07-20T21:35:20Z

Source/ASVideoPlayerNode.mm

+        for (var subnode : subnodes) {
+          [self addSubnode:subnode];
+        }
+      }


Talked to @maicki offline. Let's leave this as is since my suggested approach is a bit more complicated and in case of early return(s), the vector will not be used.

* Improve locking in ASVideoPlayerNode * Address comments

maicki requested a review from nguyenhuy July 19, 2018 16:26

Improve locking in ASVideoPlayerNode

50f37b1

maicki force-pushed the MSVideoPlayerNodeLocking branch from cefbf21 to 50f37b1 Compare July 19, 2018 16:27

maicki commented Jul 19, 2018

View reviewed changes

nguyenhuy requested changes Jul 20, 2018

View reviewed changes

Address comments

90116e3

nguyenhuy approved these changes Jul 20, 2018

View reviewed changes

maicki merged commit d9d5b12 into master Jul 20, 2018

nguyenhuy deleted the MSVideoPlayerNodeLocking branch July 20, 2018 22:43

mikezucc pushed a commit to mikezucc/Texture that referenced this pull request Oct 2, 2018

Improve locking situation in ASVideoPlayerNode (TextureGroup#1042)

94d32a6

* Improve locking in ASVideoPlayerNode * Address comments

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve locking situation in ASVideoPlayerNode #1042

Improve locking situation in ASVideoPlayerNode #1042

maicki commented Jul 19, 2018 •

edited

Loading

ghost commented Jul 19, 2018

ghost commented Jul 19, 2018

maicki Jul 19, 2018 •

edited

Loading

maicki Jul 19, 2018

nguyenhuy Jul 20, 2018

nguyenhuy left a comment

nguyenhuy Jul 20, 2018

nguyenhuy Jul 20, 2018

nguyenhuy Jul 20, 2018

nguyenhuy Jul 20, 2018

nguyenhuy Jul 20, 2018

nguyenhuy Jul 20, 2018 •

edited

Loading

nguyenhuy Jul 20, 2018

nguyenhuy Jul 20, 2018

		@@ -284,15 +282,23 @@ - (void)createControls

		if (_delegateFlags.delegateCustomControls && _delegateFlags.delegateLayoutSpecForControls) {
		NSDictionary *customControls = [_delegate videoPlayerNodeCustomControls:self];

Improve locking situation in ASVideoPlayerNode #1042

Improve locking situation in ASVideoPlayerNode #1042

Conversation

maicki commented Jul 19, 2018 • edited Loading

ghost commented Jul 19, 2018

ghost commented Jul 19, 2018

maicki Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyenhuy left a 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

nguyenhuy Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maicki commented Jul 19, 2018 •

edited

Loading

maicki Jul 19, 2018 •

edited

Loading

nguyenhuy Jul 20, 2018 •

edited

Loading