From 0494a84d989e8d8095d9b8917cffb9a68746f75b Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Thu, 2 Jan 2020 07:12:06 -0800 Subject: [PATCH] Make sure that plugins don't store events unbounded Summary: To avoid plugins to collect data forever and store it (if they are never opened), introduced a hard-coded default limit on how many events will be preserved. A semantic change is that plugins have to be potentially resistant against missing events. So far, during testing, this didn't seem to cause any problems. Reviewed By: jonathoma Differential Revision: D19175912 fbshipit-source-id: 828be941e76b7356c9f5be7e5a49de9a7a31b0d2 --- docs/extending/js-plugin-api.md | 3 ++ src/plugin.tsx | 2 ++ src/reducers/pluginMessageQueue.tsx | 35 +++++++++++++------- src/utils/__tests__/messageQueue.node.tsx | 40 +++++++++++++++++++++++ src/utils/messageQueue.tsx | 10 +++++- 5 files changed, 78 insertions(+), 12 deletions(-) diff --git a/docs/extending/js-plugin-api.md b/docs/extending/js-plugin-api.md index 8d1a0a330..b05374471 100644 --- a/docs/extending/js-plugin-api.md +++ b/docs/extending/js-plugin-api.md @@ -78,6 +78,9 @@ static persistedStateReducer( ``` The job of the `persistedStateReducer` is to merge incoming data into the state, so that next time the plugin is activated, the persisted state will be ready. +If a plugin has a `persistedStateReducer`, and the plugin is not open in flipper, incoming messages are queued until the plugin is opened. + +The amount of events that is cached for a plugin is controlled by the optional static field `maxQueueSize`, which by default is set to `10000` events. The data that is produced from `persistedStateReducer` should be immutable, but also structurally sharing unchanged parts of the state with the previous state to avoid performance hiccups. To simplify this process we recommend using the [Immer](https://immerjs.github.io/immer/docs/introduction) package. Immer makes it possible to keep the reducer concise by directly supporting "writing" to the current state, and keeping track of that in the background. diff --git a/src/plugin.tsx b/src/plugin.tsx index a28e39f51..8b4ce0f2d 100644 --- a/src/plugin.tsx +++ b/src/plugin.tsx @@ -20,6 +20,7 @@ import {Idler} from './utils/Idler'; import {StaticView} from './reducers/connections'; import {State as ReduxState} from './reducers'; import {PersistedState} from './plugins/layout'; +import {DEFAULT_MAX_QUEUE_SIZE} from './reducers/pluginMessageQueue'; type Parameters = any; // This function is intended to be called from outside of the plugin. @@ -98,6 +99,7 @@ export abstract class FlipperBasePlugin< static screenshot: string | null; static defaultPersistedState: any; static persistedStateReducer: PersistedStateReducer | null; + static maxQueueSize: number = DEFAULT_MAX_QUEUE_SIZE; static metricsReducer: | ((persistedState: StaticPersistedState) => Promise) | null; diff --git a/src/reducers/pluginMessageQueue.tsx b/src/reducers/pluginMessageQueue.tsx index 40547aab6..332893e7f 100644 --- a/src/reducers/pluginMessageQueue.tsx +++ b/src/reducers/pluginMessageQueue.tsx @@ -10,6 +10,8 @@ import produce from 'immer'; import {deconstructPluginKey} from '../utils/clientUtils'; +export const DEFAULT_MAX_QUEUE_SIZE = 10000; + export type Message = { method: string; params: any; @@ -24,6 +26,7 @@ export type Action = type: 'QUEUE_MESSAGE'; payload: { pluginKey: string; // client + plugin + maxQueueSize: number; } & Message; } | { @@ -46,16 +49,20 @@ export default function reducer( ): State { switch (action.type) { case 'QUEUE_MESSAGE': { - const {pluginKey, method, params} = action.payload; - return produce(state, draft => { - if (!draft[pluginKey]) { - draft[pluginKey] = []; - } - draft[pluginKey].push({ - method, - params, - }); - }); + const {pluginKey, method, params, maxQueueSize} = action.payload; + // this is hit very often, so try to do it a bit optimal + const currentMessages = state[pluginKey] || []; + const newMessages = + currentMessages.length < maxQueueSize + ? currentMessages.slice() + : // throw away first 10% of the queue if it gets too full + (console.log(`Dropping events for plugin ${pluginKey}`), + currentMessages.slice(Math.floor(maxQueueSize / 10))); + newMessages.push({method, params}); + return { + ...state, + [pluginKey]: newMessages, + }; } case 'CLEAR_MESSAGE_QUEUE': { @@ -94,9 +101,15 @@ export const queueMessage = ( pluginKey: string, method: string, params: any, + maxQueueSize: number | undefined, ): Action => ({ type: 'QUEUE_MESSAGE', - payload: {pluginKey, method, params}, + payload: { + pluginKey, + method, + params, + maxQueueSize: maxQueueSize || DEFAULT_MAX_QUEUE_SIZE, + }, }); export const clearMessageQueue = ( diff --git a/src/utils/__tests__/messageQueue.node.tsx b/src/utils/__tests__/messageQueue.node.tsx index 061f73201..9f20749b5 100644 --- a/src/utils/__tests__/messageQueue.node.tsx +++ b/src/utils/__tests__/messageQueue.node.tsx @@ -14,6 +14,10 @@ import {selectPlugin} from '../../reducers/connections'; import {processMessageQueue} from '../messageQueue'; import {getPluginKey} from '../pluginUtils'; import {TestIdler} from '../Idler'; +import pluginMessageQueue, { + State, + queueMessage, +} from '../../reducers/pluginMessageQueue'; interface PersistedState { count: 1; @@ -339,3 +343,39 @@ test('queue - make sure resetting plugin state clears the message queue', async }, ); }); + +test('queue will be cleaned up when it exceeds maximum size', () => { + let state: State = {}; + const pluginKey = 'test'; + const queueSize = 5000; + let i = 0; + for (i = 0; i < queueSize; i++) { + state = pluginMessageQueue( + state, + queueMessage(pluginKey, 'test', {i}, queueSize), + ); + } + // almost full + expect(state[pluginKey][0]).toEqual({method: 'test', params: {i: 0}}); + expect(state[pluginKey].length).toBe(queueSize); // ~5000 + expect(state[pluginKey][queueSize - 1]).toEqual({ + method: 'test', + params: {i: queueSize - 1}, // ~4999 + }); + + state = pluginMessageQueue( + state, + queueMessage(pluginKey, 'test', {i: ++i}, queueSize), + ); + + const newLength = Math.ceil(0.9 * queueSize) + 1; // ~4500 + expect(state[pluginKey].length).toBe(newLength); + expect(state[pluginKey][0]).toEqual({ + method: 'test', + params: {i: queueSize - newLength + 1}, // ~500 + }); + expect(state[pluginKey][newLength - 1]).toEqual({ + method: 'test', + params: {i: i}, // ~50001 + }); +}); diff --git a/src/utils/messageQueue.tsx b/src/utils/messageQueue.tsx index 5c7c5f142..da3908148 100644 --- a/src/utils/messageQueue.tsx +++ b/src/utils/messageQueue.tsx @@ -128,6 +128,7 @@ export function processMessageLater( defaultPersistedState: any; name: string; persistedStateReducer: PersistedStateReducer | null; + maxQueueSize?: number; }, message: {method: string; params?: any}, ) { @@ -149,7 +150,14 @@ export function processMessageLater( } else { // TODO: possible optimization: drop all messages for non-favorited plugins // TODO: possible optimization: drop messages if queue is too large - store.dispatch(queueMessage(pluginKey, message.method, message.params)); + store.dispatch( + queueMessage( + pluginKey, + message.method, + message.params, + plugin.maxQueueSize, + ), + ); } }