Stop connecting disabled background plugins

Summary:
Background for this diff: https://fb.quip.com/KqEfAlKYlgme

Some plugins don't respect that stuff (livefeed and graphql), but for others it seems to work fine.

This is just a PoC, there are some present bugs concerning the combination of selecting and bg plugins

Questions to investigate:

- [x] make sure that LiveFeed and GraphQL disconnect properly. There might be more plugins that need that
- [x] verifiy that we don't loose one of the original goals of background plugins, e.g. QPL collecting and sending data from device start. Does this still work as intended after this change?
- [x] how can we observe / measure improvements? Are dev builds more responsive after this? Is the layout inspector smoother for example because no QPL plugins are interweaved?
- [x] how is forward and backward compatibility?
   - If Flipper is updated, but device not: No change I think, as getBackgroundPlugins() will return an empty set, and background plugins are initiated as usual, so old behavior
  - If device is updated, but Flipper not, background plugins won't be started until they are selected. This is a degradation, but hopefully explainable.
- [x] Verify QPL buffer is not unbounded
- [x] Share architecutre changes with team

For Graphql updates: D20943455

Added runtime stats to monitor network traffic (sadly had to redo that since scuba couldn't handle the data format used at first, so probably will hold of landing this diff a week to make sure we can see some effects)

Follow up work:

[x] wait until we released the stat tracking before we release this, to be able to measure the effect?
[x] make sure graphql fix lands
[ ] use side effects abstraction
[ ] fix other background plugins (android only) or fix it in a generic way:

{F234394286}

Changelog: Background plugins will no longer receive a Flipper connection if they are disabled. This should significantly reduce the overall load of Flipper both on the device and desktop when unused plugins are disabled used, which could otherwise generate 10MB/s of network traffic certain scenarios. All plugins *should* be able to handle to this gracefully, but since this is quite a fundamental change, reach out to the Flipper team when in doubt!

Reviewed By: jknoxville

Differential Revision: D20942453

fbshipit-source-id: b699199cb95c1b3e4c36e026b6dfaee7d1652e1f

desktop/app/src/Client.tsx

@@ -114,6 +114,7 @@ export default class Client extends EventEmitter {
   sdkVersion: number;
   messageIdCounter: number;
   plugins: Plugins;
+  backgroundPlugins: Plugins;
   connection: FlipperClientConnection<any, any> | null | undefined;
   store: Store;
   activePlugins: Set<string>;
@@ -145,6 +146,7 @@ export default class Client extends EventEmitter {
     super();
     this.connected = true;
     this.plugins = plugins ? plugins : [];
+    this.backgroundPlugins = [];
     this.connection = conn;
     this.id = id;
     this.query = query;
@@ -235,13 +237,27 @@ export default class Client extends EventEmitter {
     return this.plugins.includes(Plugin.id);
   }
 
+  isBackgroundPlugin(pluginId: string) {
+    return this.backgroundPlugins.includes(pluginId);
+  }
+
   async init() {
     this.setMatchingDevice();
-    await this.getPlugins();
+    await this.loadPlugins();
+    this.backgroundPlugins = await this.getBackgroundPlugins();
+    this.backgroundPlugins.forEach((plugin) => {
+      if (
+        this.store
+          .getState()
+          .connections.userStarredPlugins[this.query.app]?.includes(plugin)
+      ) {
+        this.initPlugin(plugin);
+      }
+    });
   }
 
   // get the supported plugins
-  async getPlugins(): Promise<Plugins> {
+  async loadPlugins(): Promise<Plugins> {
     const plugins = await this.rawCall<{plugins: Plugins}>(
       'getPlugins',
       false,
@@ -265,9 +281,44 @@ export default class Client extends EventEmitter {
     return plugins;
   }
 
+  // get the supported background plugins
+  async getBackgroundPlugins(): Promise<Plugins> {
+    if (this.sdkVersion < 4) {
+      return [];
+    }
+    return await this.rawCall<{plugins: Plugins}>(
+      'getBackgroundPlugins',
+      false,
+    ).then((data) => data.plugins);
+  }
+
   // get the plugins, and update the UI
   async refreshPlugins() {
-    await this.getPlugins();
+    const oldBackgroundPlugins = this.backgroundPlugins;
+    await this.loadPlugins();
+    const newBackgroundPlugins = await this.getBackgroundPlugins();
+    this.backgroundPlugins = newBackgroundPlugins;
+    // diff the background plugin list, disconnect old, connect new ones
+    oldBackgroundPlugins.forEach((plugin) => {
+      if (
+        !newBackgroundPlugins.includes(plugin) &&
+        this.store
+          .getState()
+          .connections.userStarredPlugins[this.query.app]?.includes(plugin)
+      ) {
+        this.deinitPlugin(plugin);
+      }
+    });
+    newBackgroundPlugins.forEach((plugin) => {
+      if (
+        !oldBackgroundPlugins.includes(plugin) &&
+        this.store
+          .getState()
+          .connections.userStarredPlugins[this.query.app]?.includes(plugin)
+      ) {
+        this.initPlugin(plugin);
+      }
+    });
     this.emit('plugins-change');
   }
 

desktop/app/src/plugin.tsx

@@ -269,18 +269,25 @@ export class FlipperPlugin<
 
   _teardown() {
     // automatically unsubscribe subscriptions
+    const pluginId = this.constructor.id;
     for (const {method, callback} of this.subscriptions) {
-      this.realClient.unsubscribe(this.constructor.id, method, callback);
+      this.realClient.unsubscribe(pluginId, method, callback);
     }
     // run plugin teardown
     this.teardown();
-    if (this.realClient.connected) {
-      this.realClient.deinitPlugin(this.constructor.id);
+    if (
+      this.realClient.connected &&
+      !this.realClient.isBackgroundPlugin(pluginId)
+    ) {
+      this.realClient.deinitPlugin(pluginId);
     }
   }
 
   _init() {
-    this.realClient.initPlugin(this.constructor.id);
+    const pluginId = this.constructor.id;
+    if (!this.realClient.isBackgroundPlugin(pluginId)) {
+      this.realClient.initPlugin(pluginId);
+    }
     this.init();
   }
 }

desktop/app/src/reducers/connections.tsx

@@ -277,6 +277,9 @@ const reducer = (state: State = INITAL_STATE, action: Actions): State => {
 
     case 'STAR_PLUGIN': {
       const {selectedPlugin, selectedApp} = action.payload;
+      const client = state.clients.find(
+        (client) => client.query.app === selectedApp,
+      );
       return produce(state, (draft) => {
         if (!draft.userStarredPlugins[selectedApp]) {
           draft.userStarredPlugins[selectedApp] = [selectedPlugin];
@@ -285,8 +288,14 @@ const reducer = (state: State = INITAL_STATE, action: Actions): State => {
           const idx = plugins.indexOf(selectedPlugin);
           if (idx === -1) {
             plugins.push(selectedPlugin);
+            if (client?.isBackgroundPlugin(selectedPlugin)) {
+              client.initPlugin(selectedPlugin);
+            }
           } else {
             plugins.splice(idx, 1);
+            if (client?.isBackgroundPlugin(selectedPlugin)) {
+              client.deinitPlugin(selectedPlugin);
+            }
           }
         }
       });

desktop/app/src/utils/messageQueue.tsx

@@ -221,7 +221,11 @@ export function processMessageLater(
         ),
       );
       break;
+    default:
       // In all other cases, messages will be dropped...
+      console.warn(
+        `Received message for disabled plugin ${plugin.id}: ${message.method}, dropping..`,
+      );
   }
 }
 

docs/extending/new-clients.mdx

@@ -70,6 +70,23 @@ Response = {
 }
 ```
 
+
+### getBackgroundPlugins
+
+Returns a subset of the available plugins returned by `getPlugin`. The background connections will automatically receive a connection from Flipper once it starts (and if the plugins are enabled), rather than waiting for the user to open the plugin.
+
+```
+Request = {
+  "method": "getBackgroundPlugins",
+}
+
+Response = {
+  "success": {
+    "plugins": Array<string>
+  },
+}
+```
+
 ### init
 Initialize a plugin. This should result in an onConnected call on the appropriate plugin. Plugins should by nature be lazy and should not be initialized up front as this may incur significant cost. The Flipper desktop client knows when a plugin is needed and should control when to initialize them.
 ```

iOS/FlipperKitTests/FlipperClientTests.mm

@@ -92,7 +92,7 @@ FlipperClient* objcClient;
   XCTAssertEqual(successes[0], expected);
 }
 
-- (void)testPluginActivatedInBackgroundMode {
+- (void)testPluginNotActivatedInBackgroundMode {
   __block BOOL pluginConnected = NO;
   BlockBasedSonarPlugin* cat =
       [[BlockBasedSonarPlugin alloc] initIdentifier:@"cat"
@@ -106,7 +106,7 @@ FlipperClient* objcClient;
 
   [objcClient addPlugin:cat];
   [objcClient start];
-  XCTAssertTrue(pluginConnected);
+  XCTAssertFalse(pluginConnected);
 }
 
 - (void)testPluginNotActivatedInNonBackgroundMode {
@@ -153,7 +153,7 @@ FlipperClient* objcClient;
 }
 
 - (void)testConnectAndDisconnectCallbackForBackgroundCase {
-  __block BOOL pluginConnected = YES;
+  __block BOOL pluginConnected = NO;
   BlockBasedSonarPlugin* cat =
       [[BlockBasedSonarPlugin alloc] initIdentifier:@"cat"
           connect:^(id<FlipperConnection>) {
@@ -166,7 +166,27 @@ FlipperClient* objcClient;
 
   [objcClient addPlugin:cat];
   [objcClient start];
+  XCTAssertFalse(pluginConnected);
+
+  folly::dynamic messageInit = folly::dynamic::object("method", "init")(
+      "params", folly::dynamic::object("plugin", "cat"));
+  std::unique_ptr<facebook::flipper::FlipperResponder> responder =
+      std::make_unique<facebook::flipper::FlipperResponderMock>();
+  socket->callbacks->onMessageReceived(messageInit, std::move(responder));
   XCTAssertTrue(pluginConnected);
+
+  folly::dynamic messageDeInit = folly::dynamic::object("method", "deinit")(
+      "params", folly::dynamic::object("plugin", "cat"));
+  std::unique_ptr<facebook::flipper::FlipperResponder> responder2 =
+      std::make_unique<facebook::flipper::FlipperResponderMock>();
+  socket->callbacks->onMessageReceived(messageDeInit, std::move(responder2));
+  XCTAssertFalse(pluginConnected);
+
+  std::unique_ptr<facebook::flipper::FlipperResponder> responder3 =
+      std::make_unique<facebook::flipper::FlipperResponderMock>();
+  socket->callbacks->onMessageReceived(messageInit, std::move(responder3));
+  XCTAssertTrue(pluginConnected);
+
   [objcClient stop];
   XCTAssertFalse(pluginConnected);
 }
@@ -184,10 +204,16 @@ FlipperClient* objcClient;
       runInBackground:YES];
 
   [objcClient addPlugin:cat];
-  // Since background plugin's didconnect is called as soon as flipper client
-  // starts
   XCTAssertNoThrow([objcClient start]);
-  XCTAssertTrue(pluginConnected); // To be sure that connect block is called
+  XCTAssertFalse(pluginConnected);
+
+  folly::dynamic messageInit = folly::dynamic::object("method", "init")(
+      "params", folly::dynamic::object("plugin", "cat"));
+  std::unique_ptr<facebook::flipper::FlipperResponder> responder =
+      std::make_unique<facebook::flipper::FlipperResponderMock>();
+  XCTAssertNoThrow(
+      socket->callbacks->onMessageReceived(messageInit, std::move(responder)));
+  XCTAssertTrue(pluginConnected);
 }
 
 - (void)testCrashSuppressionInDisconnectCallback {
@@ -205,6 +231,12 @@ FlipperClient* objcClient;
   [objcClient addPlugin:cat];
   [objcClient start];
 
+  folly::dynamic messageInit = folly::dynamic::object("method", "init")(
+      "params", folly::dynamic::object("plugin", "cat"));
+  std::unique_ptr<facebook::flipper::FlipperResponder> responder =
+      std::make_unique<facebook::flipper::FlipperResponderMock>();
+  socket->callbacks->onMessageReceived(messageInit, std::move(responder));
+
   XCTAssertNoThrow(
       [objcClient stop]); // Stopping client will call disconnect of the plugin
   XCTAssertTrue(isCalled); // To be sure that connect block is called
@@ -264,13 +296,19 @@ FlipperClient* objcClient;
   [objcClient addPlugin:cat];
   [objcClient start];
 
+  folly::dynamic messageInit = folly::dynamic::object("method", "init")(
+      "params", folly::dynamic::object("plugin", "PluginIdentifier"));
+  std::unique_ptr<facebook::flipper::FlipperResponder> responder =
+      std::make_unique<facebook::flipper::FlipperResponderMock>();
+  socket->callbacks->onMessageReceived(messageInit, std::move(responder));
+
   folly::dynamic message = folly::dynamic::object("id", 1)("method", "execute")(
       "params",
       folly::dynamic::object("api", "PluginIdentifier")(
           "method", "MethodName"));
-  std::unique_ptr<facebook::flipper::FlipperResponder> responder =
+  std::unique_ptr<facebook::flipper::FlipperResponder> responder2 =
       std::make_unique<facebook::flipper::FlipperResponderMock>();
-  socket->callbacks->onMessageReceived(message, std::move(responder));
+  socket->callbacks->onMessageReceived(message, std::move(responder2));
 
   XCTAssertTrue(isCalled);
 }
@@ -296,18 +334,24 @@ FlipperClient* objcClient;
   [objcClient addPlugin:cat];
   [objcClient start];
 
+  folly::dynamic messageInit = folly::dynamic::object("method", "init")(
+      "params", folly::dynamic::object("plugin", "PluginIdentifier"));
+  std::unique_ptr<facebook::flipper::FlipperResponder> responder =
+      std::make_unique<facebook::flipper::FlipperResponderMock>();
+  socket->callbacks->onMessageReceived(messageInit, std::move(responder));
+
   folly::dynamic message = folly::dynamic::object("id", 1)("method", "execute")(
       "params",
       folly::dynamic::object("api", "PluginIdentifier")(
           "method", "MethodName"));
   std::vector<folly::dynamic> successes = std::vector<folly::dynamic>();
   std::vector<folly::dynamic> errors = std::vector<folly::dynamic>();
-  std::unique_ptr<facebook::flipper::FlipperResponderMock> responder =
+  std::unique_ptr<facebook::flipper::FlipperResponderMock> responder2 =
       std::make_unique<facebook::flipper::FlipperResponderMock>(
           &successes, &errors);
 
   XCTAssertNoThrow(
-      socket->callbacks->onMessageReceived(message, std::move(responder)));
+      socket->callbacks->onMessageReceived(message, std::move(responder2)));
   XCTAssertTrue(isCalled);
   XCTAssertEqual(successes.size(), 0);
   XCTAssertEqual(errors.size(), 1);

xplat/Flipper/FlipperClient.cpp

@@ -69,12 +69,6 @@ void FlipperClient::addPlugin(std::shared_ptr<FlipperPlugin> plugin) {
     step->complete();
     if (connected_) {
       refreshPlugins();
-      if (plugin->runInBackground()) {
-        auto& conn = connections_[plugin->identifier()];
-        conn = std::make_shared<FlipperConnectionImpl>(
-            socket_.get(), plugin->identifier());
-        plugin->didConnect(conn);
-      }
     }
   });
 }
@@ -95,27 +89,6 @@ void FlipperClient::removePlugin(std::shared_ptr<FlipperPlugin> plugin) {
   });
 }
 
-void FlipperClient::startBackgroundPlugins() {
-  std::cout << "Activating Background Plugins..." << std::endl;
-  for (std::map<std::string, std::shared_ptr<FlipperPlugin>>::iterator it =
-           plugins_.begin();
-       it != plugins_.end();
-       ++it) {
-    std::cout << it->first << std::endl;
-    if (it->second.get()->runInBackground()) {
-      try {
-        auto& conn = connections_[it->first];
-        conn =
-            std::make_shared<FlipperConnectionImpl>(socket_.get(), it->first);
-        it->second.get()->didConnect(conn);
-      } catch (std::exception& e) {
-        log("Exception starting background plugin: " + it->first + ". " +
-            e.what());
-      }
-    }
-  }
-}
-
 std::shared_ptr<FlipperPlugin> FlipperClient::getPlugin(
     const std::string& identifier) {
   std::lock_guard<std::mutex> lock(mutex_);
@@ -130,6 +103,15 @@ bool FlipperClient::hasPlugin(const std::string& identifier) {
   return plugins_.find(identifier) != plugins_.end();
 }
 
+void FlipperClient::connect(std::shared_ptr<FlipperPlugin> plugin) {
+  if (connections_.find(plugin->identifier()) == connections_.end()) {
+    auto& conn = connections_[plugin->identifier()];
+    conn = std::make_shared<FlipperConnectionImpl>(
+        socket_.get(), plugin->identifier());
+    plugin->didConnect(conn);
+  }
+}
+
 void FlipperClient::disconnect(std::shared_ptr<FlipperPlugin> plugin) {
   const auto& conn = connections_.find(plugin->identifier());
   if (conn != connections_.end()) {
@@ -151,7 +133,6 @@ void FlipperClient::onConnected() {
 
     std::lock_guard<std::mutex> lock(mutex_);
     connected_ = true;
-    startBackgroundPlugins();
   });
 }
 
@@ -189,6 +170,18 @@ void FlipperClient::onMessageReceived(
       return;
     }
 
+    if (method == "getBackgroundPlugins") {
+      dynamic identifiers = dynamic::array();
+      for (const auto& elem : plugins_) {
+        if (elem.second->runInBackground()) {
+          identifiers.push_back(elem.first);
+        }
+      }
+      dynamic response = dynamic::object("plugins", identifiers);
+      responder->success(response);
+      return;
+    }
+
     if (method == "init") {
       const auto identifier = params["plugin"].getString();
       if (plugins_.find(identifier) == plugins_.end()) {
@@ -200,12 +193,7 @@ void FlipperClient::onMessageReceived(
         return;
       }
       const auto plugin = plugins_.at(identifier);
-      if (!plugin.get()->runInBackground()) {
-        auto& conn = connections_[plugin->identifier()];
-        conn = std::make_shared<FlipperConnectionImpl>(
-            socket_.get(), plugin->identifier());
-        plugin->didConnect(conn);
-      }
+      connect(plugin);
       return;
     }
 
@@ -220,9 +208,7 @@ void FlipperClient::onMessageReceived(
         return;
       }
       const auto plugin = plugins_.at(identifier);
-      if (!plugin.get()->runInBackground()) {
       disconnect(plugin);
-      }
       return;
     }
 

xplat/Flipper/FlipperClient.h

@@ -108,8 +108,8 @@ class FlipperClient : public FlipperConnectionManager::Callbacks {
   std::mutex mutex_;
   std::shared_ptr<FlipperState> flipperState_;
 
+  void connect(std::shared_ptr<FlipperPlugin> plugin);
   void disconnect(std::shared_ptr<FlipperPlugin> plugin);
-  void startBackgroundPlugins();
   std::string callstack();
   void handleError(std::exception& e);
 };

xplat/Flipper/FlipperConnectionManagerImpl.cpp

@@ -35,7 +35,7 @@ static constexpr int maxPayloadSize = 0xFFFFFF;
 // Not a public-facing version number.
 // Used for compatibility checking with desktop flipper.
 // To be bumped for every core platform interface change.
-static constexpr int sdkVersion = 3;
+static constexpr int sdkVersion = 4;
 
 namespace facebook {
 namespace flipper {

xplat/FlipperTests/FlipperClientTests.cpp

@@ -312,6 +312,10 @@ TEST_F(FlipperClientTest, testExceptionUnknownApi) {
 }
 
 TEST_F(FlipperClientTest, testBackgroundPluginActivated) {
+  dynamic messageInit = dynamic::object("method", "init")(
+      "params", dynamic::object("plugin", "Test"));
+  dynamic messageDeinit = dynamic::object("method", "deinit")(
+      "params", dynamic::object("plugin", "Test"));
   bool pluginConnected = false;
   const auto connectionCallback = [&](std::shared_ptr<FlipperConnection> conn) {
     pluginConnected = true;
@@ -322,7 +326,26 @@ TEST_F(FlipperClientTest, testBackgroundPluginActivated) {
 
   client->addPlugin(plugin);
   client->start();
+  EXPECT_FALSE(pluginConnected);
+
+  {
+    auto responder = std::make_unique<FlipperResponderMock>();
+    socket->onMessageReceived(messageInit, getResponder());
     EXPECT_TRUE(pluginConnected);
+  }
+
+  {
+    auto responder = std::make_unique<FlipperResponderMock>();
+    socket->onMessageReceived(messageDeinit, getResponder());
+    EXPECT_FALSE(pluginConnected);
+  }
+
+  {
+    auto responder = std::make_unique<FlipperResponderMock>();
+    socket->onMessageReceived(messageInit, getResponder());
+    EXPECT_TRUE(pluginConnected);
+  }
+
   client->stop();
   EXPECT_FALSE(pluginConnected);
 }