From 952e9296990478bb60492f31dce4f416920faf56 Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Wed, 1 Jul 2020 08:58:40 -0700 Subject: [PATCH] Fix disabling a plugin nog clearing the message queue Summary: While writing unit tests discovered a bug that disabling a plugin doesn't guarantee cleaning the messagequeues (both the buffer in client and the messagequeue reducer). Fixed that. That was thanks to @#%@#$@#%@ Redux a lot harder than it should be; as 'STAR_PLUGIN' reasons about a plugin + app name, while the message queue reducer would need to deduct the plugin keys from that, but it can't because that mapping is stored in the connections reducers. So I moved the `STAR_PLUGIN` action handling to the root reducer, sot that it can reason about the state of multiple reducers, which looked like the least of all evils. For more ranting about that and possible alternative solutions: https://twitter.com/mweststrate/status/1277556309706117122 Reviewed By: nikoant Differential Revision: D22284043 fbshipit-source-id: 35d0a8ba3a21a5959d2bb6ef17da3ff5077f48fd --- desktop/app/src/Client.tsx | 6 ++ desktop/app/src/reducers/connections.tsx | 46 +------------- desktop/app/src/store.tsx | 63 ++++++++++++++++++- .../createMockFlipperWithPlugin.tsx | 5 +- .../src/utils/__tests__/messageQueue.node.tsx | 58 ++++++++++++++++- .../__tests__/messageQueueSandy.node.tsx | 63 ++++++++++++++++++- 6 files changed, 187 insertions(+), 54 deletions(-) diff --git a/desktop/app/src/Client.tsx b/desktop/app/src/Client.tsx index 6d768ba7c..b216a0354 100644 --- a/desktop/app/src/Client.tsx +++ b/desktop/app/src/Client.tsx @@ -336,6 +336,12 @@ export default class Client extends EventEmitter { } stopPluginIfNeeded(pluginId: string) { + const pluginKey = getPluginKey( + this.id, + {serial: this.query.device_id}, + pluginId, + ); + delete this.messageBuffer[pluginKey]; const instance = this.sandyPluginStates.get(pluginId); if (instance) { instance.destroy(); diff --git a/desktop/app/src/reducers/connections.tsx b/desktop/app/src/reducers/connections.tsx index 03c66f385..188f05b2b 100644 --- a/desktop/app/src/reducers/connections.tsx +++ b/desktop/app/src/reducers/connections.tsx @@ -135,6 +135,7 @@ export type Action = payload: number; } | { + // Implemented by rootReducer in `store.tsx` type: 'STAR_PLUGIN'; payload: { selectedApp: string; @@ -277,51 +278,6 @@ export default (state: State = INITAL_STATE, action: Actions): State => { }); } - case 'STAR_PLUGIN': { - const {plugin, selectedApp} = action.payload; - const selectedPlugin = plugin.id; - const clients = state.clients.filter( - (client) => client.query.app === selectedApp, - ); - return produce(state, (draft) => { - if (!draft.userStarredPlugins[selectedApp]) { - draft.userStarredPlugins[selectedApp] = []; - } - const plugins = draft.userStarredPlugins[selectedApp]; - const idx = plugins.indexOf(selectedPlugin); - if (idx === -1) { - plugins.push(selectedPlugin); - // enabling a plugin on one device enables it on all... - clients.forEach((client) => { - // sandy plugin? initialize it - client.startPluginIfNeeded(plugin, true); - // background plugin? connect it needed - if ( - !defaultEnabledBackgroundPlugins.includes(selectedPlugin) && - client?.isBackgroundPlugin(selectedPlugin) - ) { - client.initPlugin(selectedPlugin); - } - }); - } else { - plugins.splice(idx, 1); - // enabling a plugin on one device disables it on all... - clients.forEach((client) => { - // disconnect background plugins - if ( - !defaultEnabledBackgroundPlugins.includes(selectedPlugin) && - client?.isBackgroundPlugin(selectedPlugin) - ) { - client.deinitPlugin(selectedPlugin); - } - // stop sandy plugins - // TODO: forget any persisted state as well T68683449 - client.stopPluginIfNeeded(plugin.id); - }); - } - }); - } - case 'SELECT_USER_PREFERRED_PLUGIN': { const {payload} = action; return {...state, userPreferredPlugin: payload}; diff --git a/desktop/app/src/store.tsx b/desktop/app/src/store.tsx index f6186360a..d73d8cefc 100644 --- a/desktop/app/src/store.tsx +++ b/desktop/app/src/store.tsx @@ -11,9 +11,14 @@ import {createStore} from 'redux'; import reducers, {Actions, State as StoreState} from './reducers/index'; import {stateSanitizer} from './utils/reduxDevToolsConfig'; import isProduction from './utils/isProduction'; +import produce from 'immer'; +import { + defaultEnabledBackgroundPlugins, + getPluginKey, +} from './utils/pluginUtils'; export const store = createStore( - reducers, + rootReducer, // @ts-ignore Type definition mismatch window.__REDUX_DEVTOOLS_EXTENSION__ ? window.__REDUX_DEVTOOLS_EXTENSION__({ @@ -23,6 +28,62 @@ export const store = createStore( : undefined, ); +export function rootReducer( + state: StoreState | undefined, + action: Actions, +): StoreState { + if (action.type === 'STAR_PLUGIN' && state) { + const {plugin, selectedApp} = action.payload; + const selectedPlugin = plugin.id; + const clients = state.connections.clients.filter( + (client) => client.query.app === selectedApp, + ); + return produce(state, (draft) => { + if (!draft.connections.userStarredPlugins[selectedApp]) { + draft.connections.userStarredPlugins[selectedApp] = []; + } + const plugins = draft.connections.userStarredPlugins[selectedApp]; + const idx = plugins.indexOf(selectedPlugin); + if (idx === -1) { + plugins.push(selectedPlugin); + // enabling a plugin on one device enables it on all... + clients.forEach((client) => { + // sandy plugin? initialize it + client.startPluginIfNeeded(plugin, true); + // background plugin? connect it needed + if ( + !defaultEnabledBackgroundPlugins.includes(selectedPlugin) && + client?.isBackgroundPlugin(selectedPlugin) + ) { + client.initPlugin(selectedPlugin); + } + }); + } else { + plugins.splice(idx, 1); + // enabling a plugin on one device disables it on all... + clients.forEach((client) => { + // disconnect background plugins + if ( + !defaultEnabledBackgroundPlugins.includes(selectedPlugin) && + client?.isBackgroundPlugin(selectedPlugin) + ) { + client.deinitPlugin(selectedPlugin); + } + // stop sandy plugins + // TODO: forget any persisted state as well T68683449 + client.stopPluginIfNeeded(plugin.id); + delete draft.pluginMessageQueue[ + getPluginKey(client.id, {serial: client.query.device_id}, plugin.id) + ]; + }); + } + }); + } + + // otherwise + return reducers(state, action); +} + if (!isProduction()) { // For debugging purposes only // @ts-ignore diff --git a/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx b/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx index 9a1253d17..d9af3ad90 100644 --- a/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx +++ b/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx @@ -24,7 +24,8 @@ import { } from '../reducers/connections'; import BaseDevice from '../devices/BaseDevice'; -import reducers, {Store} from '../reducers/index'; +import {rootReducer} from '../store'; +import {Store} from '../reducers/index'; import Client, {ClientQuery} from '../Client'; import {buildClientId} from '../utils/clientUtils'; @@ -58,7 +59,7 @@ export async function createMockFlipperWithPlugin( pluginClazz: PluginDefinition, options?: MockOptions, ): Promise { - const store = createStore(reducers); + const store = createStore(rootReducer); const logger = getInstance(); store.dispatch(registerPlugins([pluginClazz])); diff --git a/desktop/app/src/utils/__tests__/messageQueue.node.tsx b/desktop/app/src/utils/__tests__/messageQueue.node.tsx index 585605ae9..fe69a0a7b 100644 --- a/desktop/app/src/utils/__tests__/messageQueue.node.tsx +++ b/desktop/app/src/utils/__tests__/messageQueue.node.tsx @@ -190,9 +190,7 @@ test('queue - events are NOT processed immediately if plugin is NOT selected (bu selectDeviceLogs(store); sendMessage('inc', {delta: 4}); client.flushMessageBuffer(); - expect(store.getState().pluginMessageQueue).toEqual({ - [pluginKey]: [], - }); + expect(store.getState().pluginMessageQueue).toEqual({}); // star again, plugin still not selected, message is queued starTestPlugin(store, client); @@ -646,3 +644,57 @@ test('client - incoming messages are buffered and flushed together', async () => } `); }); + +test('queue - messages that have not yet flushed be lost when disabling the plugin', async () => { + const { + client, + store, + sendMessage, + pluginKey, + } = await createMockFlipperWithPlugin(TestPlugin); + selectDeviceLogs(store); + + sendMessage('inc', {}); + sendMessage('inc', {delta: 2}); + + expect(client.messageBuffer).toMatchInlineSnapshot(` + Object { + "TestApp#Android#MockAndroidDevice#serial#TestPlugin": Object { + "messages": Array [ + Object { + "api": "TestPlugin", + "method": "inc", + "params": Object { + "delta": 2, + }, + }, + ], + "plugin": [Function], + }, + } + `); + expect(store.getState().pluginMessageQueue).toMatchInlineSnapshot(` + Object { + "TestApp#Android#MockAndroidDevice#serial#TestPlugin": Array [ + Object { + "api": "TestPlugin", + "method": "inc", + "params": Object {}, + }, + ], + } + `); + + // disable + starTestPlugin(store, client); + expect(client.messageBuffer).toMatchInlineSnapshot(`Object {}`); + expect(store.getState().pluginMessageQueue).toMatchInlineSnapshot( + `Object {}`, + ); + + // re-enable, no messages arrive + starTestPlugin(store, client); + client.flushMessageBuffer(); + processMessageQueue(TestPlugin, pluginKey, store); + expect(store.getState().pluginStates).toMatchInlineSnapshot(`Object {}`); +}); diff --git a/desktop/app/src/utils/__tests__/messageQueueSandy.node.tsx b/desktop/app/src/utils/__tests__/messageQueueSandy.node.tsx index bcedee7a4..002a492e4 100644 --- a/desktop/app/src/utils/__tests__/messageQueueSandy.node.tsx +++ b/desktop/app/src/utils/__tests__/messageQueueSandy.node.tsx @@ -197,9 +197,8 @@ test('queue - events are NOT processed immediately if plugin is NOT selected (bu selectDeviceLogs(store); sendMessage('inc', {delta: 4}); client.flushMessageBuffer(); - expect(store.getState().pluginMessageQueue).toEqual({ - [pluginKey]: [], - }); + expect(client.messageBuffer).toMatchInlineSnapshot(`Object {}`); + expect(store.getState().pluginMessageQueue).toEqual({}); // star again, plugin still not selected, message is queued starTestPlugin(store, client); @@ -637,3 +636,61 @@ test('client - incoming messages are buffered and flushed together', async () => } `); }); + +test('queue - messages that have not yet flushed be lost when disabling the plugin', async () => { + const { + client, + store, + sendMessage, + pluginKey, + } = await createMockFlipperWithPlugin(TestPlugin); + selectDeviceLogs(store); + + sendMessage('inc', {}); + sendMessage('inc', {delta: 2}); + + expect(client.messageBuffer).toMatchInlineSnapshot(` + Object { + "TestApp#Android#MockAndroidDevice#serial#TestPlugin": Object { + "messages": Array [ + Object { + "api": "TestPlugin", + "method": "inc", + "params": Object { + "delta": 2, + }, + }, + ], + "plugin": undefined, + }, + } + `); + expect(store.getState().pluginMessageQueue).toMatchInlineSnapshot(` + Object { + "TestApp#Android#MockAndroidDevice#serial#TestPlugin": Array [ + Object { + "api": "TestPlugin", + "method": "inc", + "params": Object {}, + }, + ], + } + `); + + // disable + starTestPlugin(store, client); + expect(client.messageBuffer).toMatchInlineSnapshot(`Object {}`); + expect(store.getState().pluginMessageQueue).toMatchInlineSnapshot( + `Object {}`, + ); + + // re-enable, no messages arrive + starTestPlugin(store, client); + client.flushMessageBuffer(); + processMessageQueue( + client.sandyPluginStates.get(TestPlugin.id)!, + pluginKey, + store, + ); + expect(getTestPluginState(client)).toEqual({count: 0}); +});