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
This commit is contained in:
Michel Weststrate
2020-01-02 07:12:06 -08:00
committed by Facebook Github Bot
parent 8c8f360572
commit 0494a84d98
5 changed files with 78 additions and 12 deletions

View File

@@ -78,6 +78,9 @@ static persistedStateReducer<PersistedState>(
``` ```
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. 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. 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. 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.

View File

@@ -20,6 +20,7 @@ import {Idler} from './utils/Idler';
import {StaticView} from './reducers/connections'; import {StaticView} from './reducers/connections';
import {State as ReduxState} from './reducers'; import {State as ReduxState} from './reducers';
import {PersistedState} from './plugins/layout'; import {PersistedState} from './plugins/layout';
import {DEFAULT_MAX_QUEUE_SIZE} from './reducers/pluginMessageQueue';
type Parameters = any; type Parameters = any;
// This function is intended to be called from outside of the plugin. // 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 screenshot: string | null;
static defaultPersistedState: any; static defaultPersistedState: any;
static persistedStateReducer: PersistedStateReducer | null; static persistedStateReducer: PersistedStateReducer | null;
static maxQueueSize: number = DEFAULT_MAX_QUEUE_SIZE;
static metricsReducer: static metricsReducer:
| ((persistedState: StaticPersistedState) => Promise<MetricType>) | ((persistedState: StaticPersistedState) => Promise<MetricType>)
| null; | null;

View File

@@ -10,6 +10,8 @@
import produce from 'immer'; import produce from 'immer';
import {deconstructPluginKey} from '../utils/clientUtils'; import {deconstructPluginKey} from '../utils/clientUtils';
export const DEFAULT_MAX_QUEUE_SIZE = 10000;
export type Message = { export type Message = {
method: string; method: string;
params: any; params: any;
@@ -24,6 +26,7 @@ export type Action =
type: 'QUEUE_MESSAGE'; type: 'QUEUE_MESSAGE';
payload: { payload: {
pluginKey: string; // client + plugin pluginKey: string; // client + plugin
maxQueueSize: number;
} & Message; } & Message;
} }
| { | {
@@ -46,16 +49,20 @@ export default function reducer(
): State { ): State {
switch (action.type) { switch (action.type) {
case 'QUEUE_MESSAGE': { case 'QUEUE_MESSAGE': {
const {pluginKey, method, params} = action.payload; const {pluginKey, method, params, maxQueueSize} = action.payload;
return produce(state, draft => { // this is hit very often, so try to do it a bit optimal
if (!draft[pluginKey]) { const currentMessages = state[pluginKey] || [];
draft[pluginKey] = []; const newMessages =
} currentMessages.length < maxQueueSize
draft[pluginKey].push({ ? currentMessages.slice()
method, : // throw away first 10% of the queue if it gets too full
params, (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': { case 'CLEAR_MESSAGE_QUEUE': {
@@ -94,9 +101,15 @@ export const queueMessage = (
pluginKey: string, pluginKey: string,
method: string, method: string,
params: any, params: any,
maxQueueSize: number | undefined,
): Action => ({ ): Action => ({
type: 'QUEUE_MESSAGE', type: 'QUEUE_MESSAGE',
payload: {pluginKey, method, params}, payload: {
pluginKey,
method,
params,
maxQueueSize: maxQueueSize || DEFAULT_MAX_QUEUE_SIZE,
},
}); });
export const clearMessageQueue = ( export const clearMessageQueue = (

View File

@@ -14,6 +14,10 @@ import {selectPlugin} from '../../reducers/connections';
import {processMessageQueue} from '../messageQueue'; import {processMessageQueue} from '../messageQueue';
import {getPluginKey} from '../pluginUtils'; import {getPluginKey} from '../pluginUtils';
import {TestIdler} from '../Idler'; import {TestIdler} from '../Idler';
import pluginMessageQueue, {
State,
queueMessage,
} from '../../reducers/pluginMessageQueue';
interface PersistedState { interface PersistedState {
count: 1; 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
});
});

View File

@@ -128,6 +128,7 @@ export function processMessageLater(
defaultPersistedState: any; defaultPersistedState: any;
name: string; name: string;
persistedStateReducer: PersistedStateReducer | null; persistedStateReducer: PersistedStateReducer | null;
maxQueueSize?: number;
}, },
message: {method: string; params?: any}, message: {method: string; params?: any},
) { ) {
@@ -149,7 +150,14 @@ export function processMessageLater(
} else { } else {
// TODO: possible optimization: drop all messages for non-favorited plugins // TODO: possible optimization: drop all messages for non-favorited plugins
// TODO: possible optimization: drop messages if queue is too large // 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,
),
);
} }
} }