From 89d6dfcf95723e98371d9a4c5bd746eb44bce30f Mon Sep 17 00:00:00 2001
From: Adam Ernst <adamjernst@fb.com>
Date: Mon, 16 Mar 2020 07:24:41 -0700
Subject: [PATCH] Simplify invalidate batching in Layout plugin

Summary:
- Avoid holding lock while calling out to `send:withParams:`; it's hard to reason about whether we could trigger deadlock.
- Remove unnecessary `lastInvalidateMessage` ivar; we already ensure that we send at most one invalidate batch per 500ms by using `invalidateMessageQueued`.
- Misc code style items

Reviewed By: Andrey-Mishanin

Differential Revision: D20462193

fbshipit-source-id: 80f61e5a7ce5021e16ebc19a2ec40adfc46f9b92
---
 .../FlipperKitLayoutPlugin.mm                 | 49 ++++++++-----------
 1 file changed, 21 insertions(+), 28 deletions(-)

diff --git a/iOS/Plugins/FlipperKitLayoutPlugin/FlipperKitLayoutPlugin/FlipperKitLayoutPlugin.mm b/iOS/Plugins/FlipperKitLayoutPlugin/FlipperKitLayoutPlugin/FlipperKitLayoutPlugin.mm
index d64db0b0d..bb958fdc3 100644
--- a/iOS/Plugins/FlipperKitLayoutPlugin/FlipperKitLayoutPlugin/FlipperKitLayoutPlugin.mm
+++ b/iOS/Plugins/FlipperKitLayoutPlugin/FlipperKitLayoutPlugin/FlipperKitLayoutPlugin.mm
@@ -24,9 +24,8 @@
   NSMapTable<NSString*, id>* _trackedObjects;
   NSString* _lastHighlightedNode;
   NSMutableSet* _invalidObjects;
-  Boolean _invalidateMessageQueued;
-  NSDate* _lastInvalidateMessage;
-  std::mutex invalidObjectsMutex;
+  BOOL _invalidateMessageQueued;
+  std::mutex _invalidObjectsMutex;
 
   id<NSObject> _rootNode;
   id<SKTapListener> _tapListener;
@@ -49,10 +48,7 @@
   if (self = [super init]) {
     _descriptorMapper = mapper;
     _trackedObjects = [NSMapTable strongToWeakObjectsMapTable];
-    _lastHighlightedNode = nil;
     _invalidObjects = [NSMutableSet new];
-    _invalidateMessageQueued = false;
-    _lastInvalidateMessage = [NSDate date];
     _rootNode = rootNode;
     _tapListener = tapListener;
 
@@ -399,36 +395,33 @@
   [descriptor invalidateNode:node];
 
   // Collect invalidate messages before sending in a batch
-  std::lock_guard<std::mutex> lock(invalidObjectsMutex);
+  std::lock_guard<std::mutex> lock(_invalidObjectsMutex);
   [_invalidObjects addObject:nodeId];
   if (_invalidateMessageQueued) {
     return;
   }
-  _invalidateMessageQueued = true;
+  _invalidateMessageQueued = YES;
 
-  if (_lastInvalidateMessage.timeIntervalSinceNow < -1) {
-    dispatch_after(
-        dispatch_time(DISPATCH_TIME_NOW, 500 * NSEC_PER_MSEC),
-        dispatch_get_main_queue(),
-        ^{
-          [self reportInvalidatedObjects];
-        });
-  }
+  dispatch_after(
+      dispatch_time(DISPATCH_TIME_NOW, 500 * NSEC_PER_MSEC),
+      dispatch_get_main_queue(),
+      ^{
+        [self _reportInvalidatedObjects];
+      });
 }
 
-- (void)reportInvalidatedObjects {
-  std::lock_guard<std::mutex> lock(invalidObjectsMutex);
+- (void)_reportInvalidatedObjects {
   NSMutableArray* nodes = [NSMutableArray new];
-  for (NSString* nodeId in self->_invalidObjects) {
-    [nodes addObject:[NSDictionary dictionaryWithObject:nodeId forKey:@"id"]];
-  }
-  [self->_connection send:@"invalidate"
-               withParams:[NSDictionary dictionaryWithObject:nodes
-                                                      forKey:@"nodes"]];
-  self->_lastInvalidateMessage = [NSDate date];
-  self->_invalidObjects = [NSMutableSet new];
-  self->_invalidateMessageQueued = false;
-  return;
+  { // scope mutex acquisition
+    std::lock_guard<std::mutex> lock(_invalidObjectsMutex);
+    for (NSString* nodeId in _invalidObjects) {
+      [nodes addObject:@{@"id" : nodeId}];
+    }
+    _invalidObjects = [NSMutableSet new];
+    _invalidateMessageQueued = NO;
+  } // release mutex before calling out to other code
+
+  [_connection send:@"invalidate" withParams:@{@"nodes" : nodes}];
 }
 
 - (void)updateNodeReference:(id<NSObject>)node {