From b9c3d99f449f3036217445bf19b6e9143462ba4f Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Mon, 27 Apr 2020 09:43:43 -0700 Subject: [PATCH] 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 | 57 ++++++++++++++++- desktop/app/src/plugin.tsx | 15 +++-- desktop/app/src/reducers/connections.tsx | 9 +++ desktop/app/src/utils/messageQueue.tsx | 6 +- docs/extending/new-clients.mdx | 17 +++++ iOS/FlipperKitTests/FlipperClientTests.mm | 64 ++++++++++++++++--- xplat/Flipper/FlipperClient.cpp | 60 +++++++---------- xplat/Flipper/FlipperClient.h | 2 +- .../Flipper/FlipperConnectionManagerImpl.cpp | 2 +- xplat/FlipperTests/FlipperClientTests.cpp | 25 +++++++- 10 files changed, 199 insertions(+), 58 deletions(-) diff --git a/desktop/app/src/Client.tsx b/desktop/app/src/Client.tsx index 24b4f0f6d..dd07950e5 100644 --- a/desktop/app/src/Client.tsx +++ b/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 | null | undefined; store: Store; activePlugins: Set; @@ -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 { + async loadPlugins(): Promise { 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 { + 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'); } diff --git a/desktop/app/src/plugin.tsx b/desktop/app/src/plugin.tsx index 1cb12ea0a..ebd240dc8 100644 --- a/desktop/app/src/plugin.tsx +++ b/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(); } } diff --git a/desktop/app/src/reducers/connections.tsx b/desktop/app/src/reducers/connections.tsx index 1f10ca7ce..876339342 100644 --- a/desktop/app/src/reducers/connections.tsx +++ b/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); + } } } }); diff --git a/desktop/app/src/utils/messageQueue.tsx b/desktop/app/src/utils/messageQueue.tsx index 0ec940bb7..1c7e4f3a1 100644 --- a/desktop/app/src/utils/messageQueue.tsx +++ b/desktop/app/src/utils/messageQueue.tsx @@ -221,7 +221,11 @@ export function processMessageLater( ), ); break; - // In all other cases, messages will be dropped... + default: + // In all other cases, messages will be dropped... + console.warn( + `Received message for disabled plugin ${plugin.id}: ${message.method}, dropping..`, + ); } } diff --git a/docs/extending/new-clients.mdx b/docs/extending/new-clients.mdx index e30545ea3..d482c787a 100644 --- a/docs/extending/new-clients.mdx +++ b/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 + }, +} +``` + ### 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. ``` diff --git a/iOS/FlipperKitTests/FlipperClientTests.mm b/iOS/FlipperKitTests/FlipperClientTests.mm index c87129891..45cdc1004 100644 --- a/iOS/FlipperKitTests/FlipperClientTests.mm +++ b/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) { @@ -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 responder = + std::make_unique(); + 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 responder2 = + std::make_unique(); + socket->callbacks->onMessageReceived(messageDeInit, std::move(responder2)); + XCTAssertFalse(pluginConnected); + + std::unique_ptr responder3 = + std::make_unique(); + 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 responder = + std::make_unique(); + 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 responder = + std::make_unique(); + 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 responder = + std::make_unique(); + 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 responder = + std::unique_ptr responder2 = std::make_unique(); - 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 responder = + std::make_unique(); + 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 successes = std::vector(); std::vector errors = std::vector(); - std::unique_ptr responder = + std::unique_ptr responder2 = std::make_unique( &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); diff --git a/xplat/Flipper/FlipperClient.cpp b/xplat/Flipper/FlipperClient.cpp index dc3325e54..6b897d581 100644 --- a/xplat/Flipper/FlipperClient.cpp +++ b/xplat/Flipper/FlipperClient.cpp @@ -69,12 +69,6 @@ void FlipperClient::addPlugin(std::shared_ptr plugin) { step->complete(); if (connected_) { refreshPlugins(); - if (plugin->runInBackground()) { - auto& conn = connections_[plugin->identifier()]; - conn = std::make_shared( - socket_.get(), plugin->identifier()); - plugin->didConnect(conn); - } } }); } @@ -95,27 +89,6 @@ void FlipperClient::removePlugin(std::shared_ptr plugin) { }); } -void FlipperClient::startBackgroundPlugins() { - std::cout << "Activating Background Plugins..." << std::endl; - for (std::map>::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(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 FlipperClient::getPlugin( const std::string& identifier) { std::lock_guard 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 plugin) { + if (connections_.find(plugin->identifier()) == connections_.end()) { + auto& conn = connections_[plugin->identifier()]; + conn = std::make_shared( + socket_.get(), plugin->identifier()); + plugin->didConnect(conn); + } +} + void FlipperClient::disconnect(std::shared_ptr plugin) { const auto& conn = connections_.find(plugin->identifier()); if (conn != connections_.end()) { @@ -151,7 +133,6 @@ void FlipperClient::onConnected() { std::lock_guard 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( - 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); - } + disconnect(plugin); return; } diff --git a/xplat/Flipper/FlipperClient.h b/xplat/Flipper/FlipperClient.h index 1c107d6a6..28071e818 100644 --- a/xplat/Flipper/FlipperClient.h +++ b/xplat/Flipper/FlipperClient.h @@ -108,8 +108,8 @@ class FlipperClient : public FlipperConnectionManager::Callbacks { std::mutex mutex_; std::shared_ptr flipperState_; + void connect(std::shared_ptr plugin); void disconnect(std::shared_ptr plugin); - void startBackgroundPlugins(); std::string callstack(); void handleError(std::exception& e); }; diff --git a/xplat/Flipper/FlipperConnectionManagerImpl.cpp b/xplat/Flipper/FlipperConnectionManagerImpl.cpp index 288e9825a..c10fab364 100644 --- a/xplat/Flipper/FlipperConnectionManagerImpl.cpp +++ b/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 { diff --git a/xplat/FlipperTests/FlipperClientTests.cpp b/xplat/FlipperTests/FlipperClientTests.cpp index e94db2388..c855751d1 100644 --- a/xplat/FlipperTests/FlipperClientTests.cpp +++ b/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 conn) { pluginConnected = true; @@ -322,7 +326,26 @@ TEST_F(FlipperClientTest, testBackgroundPluginActivated) { client->addPlugin(plugin); client->start(); - EXPECT_TRUE(pluginConnected); + EXPECT_FALSE(pluginConnected); + + { + auto responder = std::make_unique(); + socket->onMessageReceived(messageInit, getResponder()); + EXPECT_TRUE(pluginConnected); + } + + { + auto responder = std::make_unique(); + socket->onMessageReceived(messageDeinit, getResponder()); + EXPECT_FALSE(pluginConnected); + } + + { + auto responder = std::make_unique(); + socket->onMessageReceived(messageInit, getResponder()); + EXPECT_TRUE(pluginConnected); + } + client->stop(); EXPECT_FALSE(pluginConnected); }