Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Endpoint] Fix saga to ensure they are started only after store is created #55245

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,13 @@ import { appStoreFactory } from './store';
export function renderApp(coreStart: CoreStart, { appBasePath, element }: AppMountParameters) {
coreStart.http.get('/api/endpoint/hello-world');

const store = appStoreFactory(coreStart);
const [store, stopSagas] = appStoreFactory(coreStart);

ReactDOM.render(<AppRoot basename={appBasePath} store={store} />, element);

return () => {
ReactDOM.unmountComponentAtNode(element);
stopSagas();
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,21 @@
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
import { createSagaMiddleware, SagaContext } from './index';
import { applyMiddleware, createStore, Reducer } from 'redux';

import { createSagaMiddleware, SagaContext, SagaMiddleware } from './index';
import { applyMiddleware, createStore, Reducer, Store } from 'redux';

describe('saga', () => {
const INCREMENT_COUNTER = 'INCREMENT';
const DELAYED_INCREMENT_COUNTER = 'DELAYED INCREMENT COUNTER';
const STOP_SAGA_PROCESSING = 'BREAK ASYNC ITERATOR';

const sleep = (ms = 1000) => new Promise(resolve => setTimeout(resolve, ms));
const sleep = (ms = 10) => new Promise(resolve => setTimeout(resolve, ms));
let store: Store;
let reducerA: Reducer;
let sideAffect: (a: unknown, s: unknown) => void;
let sagaExe: (sagaContext: SagaContext) => Promise<void>;
let sagaExeReduxMiddleware: SagaMiddleware;

beforeEach(() => {
reducerA = jest.fn((prevState = { count: 0 }, { type }) => {
Expand Down Expand Up @@ -47,53 +50,63 @@ describe('saga', () => {
}
}
});

sagaExeReduxMiddleware = createSagaMiddleware(sagaExe);
store = createStore(reducerA, applyMiddleware(sagaExeReduxMiddleware));
});

test('it returns Redux Middleware from createSagaMiddleware()', () => {
const sagaMiddleware = createSagaMiddleware(async () => {});
expect(sagaMiddleware).toBeInstanceOf(Function);
afterEach(() => {
sagaExeReduxMiddleware.stop();
});

test('it does nothing if saga is not started', () => {
const store = createStore(reducerA, applyMiddleware(createSagaMiddleware(sagaExe)));
expect(store.getState().count).toEqual(0);
expect(reducerA).toHaveBeenCalled();
expect(sagaExe).toHaveBeenCalled();
expect(sideAffect).not.toHaveBeenCalled();
expect(store.getState()).toEqual({ count: 0 });
expect(sagaExe).not.toHaveBeenCalled();
});
test('it updates store once running', async () => {
const sagaMiddleware = createSagaMiddleware(sagaExe);
const store = createStore(reducerA, applyMiddleware(sagaMiddleware));

test('it can dispatch store actions once running', async () => {
sagaExeReduxMiddleware.start();
expect(store.getState()).toEqual({ count: 0 });
expect(sagaExe).toHaveBeenCalled();

store.dispatch({ type: DELAYED_INCREMENT_COUNTER });
expect(store.getState()).toEqual({ count: 0 });

await sleep(100);
await sleep();

expect(sideAffect).toHaveBeenCalled();
expect(store.getState()).toEqual({ count: 1 });
});
test('it stops processing if break out of loop', async () => {
const sagaMiddleware = createSagaMiddleware(sagaExe);
const store = createStore(reducerA, applyMiddleware(sagaMiddleware));

test('it stops processing if break out of loop', async () => {
sagaExeReduxMiddleware.start();
store.dispatch({ type: DELAYED_INCREMENT_COUNTER });
await sleep(100);
await sleep();

expect(store.getState()).toEqual({ count: 1 });
expect(sideAffect).toHaveBeenCalledTimes(2);

store.dispatch({ type: STOP_SAGA_PROCESSING });
await sleep(100);
await sleep();

store.dispatch({ type: DELAYED_INCREMENT_COUNTER });
await sleep();

expect(store.getState()).toEqual({ count: 1 });
expect(sideAffect).toHaveBeenCalledTimes(2);
});

test('it stops saga middleware when stop() is called', async () => {
sagaExeReduxMiddleware.start();
store.dispatch({ type: DELAYED_INCREMENT_COUNTER });
await sleep();

expect(store.getState()).toEqual({ count: 1 });
expect(sideAffect).toHaveBeenCalledTimes(2);

sagaExeReduxMiddleware.stop();

store.dispatch({ type: DELAYED_INCREMENT_COUNTER });
await sleep(100);
await sleep();

expect(store.getState()).toEqual({ count: 1 });
expect(sideAffect).toHaveBeenCalledTimes(2);
Expand Down
46 changes: 41 additions & 5 deletions x-pack/plugins/endpoint/public/applications/endpoint/lib/saga.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,18 @@ export interface SagaContext<TAction extends AnyAction = AnyAction> {
dispatch: Dispatch<TAction>;
}

export interface SagaMiddleware extends Middleware {
/**
* Start the saga. Should be called after the `store` has been created
*/
start: () => void;

/**
* Stop the saga by exiting the internal generator `for await...of` loop.
*/
stop: () => void;
}

const noop = () => {};

/**
Expand All @@ -43,7 +55,7 @@ const noop = () => {};
* @param {Saga} saga The `saga` should initialize a long-running `for await...of` loop against
* the return value of the `actionsAndState()` method provided by the `SagaContext`.
*
* @return {Middleware}
* @return {SagaMiddleware}
*
* @example
*
Expand All @@ -64,22 +76,33 @@ const noop = () => {};
* //....
* const store = createStore(reducers, [ endpointsSagaMiddleware ]);
*/
export function createSagaMiddleware(saga: Saga): Middleware {
export function createSagaMiddleware(saga: Saga): SagaMiddleware {
const iteratorInstances = new Set<IteratorInstance>();
let runSaga: () => void = noop;
let running = false;

async function* getActionsAndStateIterator(): StoreActionsAndState {
const instance: IteratorInstance = { queue: [], nextResolve: null };
iteratorInstances.add(instance);
running = true;

try {
while (true) {
yield await nextActionAndState();
while (running) {
const actionAndState = await nextActionAndState();

// while waiting for the nextActionAndState, the saga could have been stopped.
if (!running) {
continue;
}

yield actionAndState;
}
} finally {
// If the consumer stops consuming this (e.g. `break` or `return` is called in the `for await`
// then this `finally` block will run and unregister this instance and reset `runSaga`
iteratorInstances.delete(instance);
runSaga = noop;
running = false;
}

function nextActionAndState() {
Expand Down Expand Up @@ -109,7 +132,6 @@ export function createSagaMiddleware(saga: Saga): Middleware {
actionsAndState: getActionsAndStateIterator,
dispatch,
});
runSaga();
}
return (next: Dispatch<AnyAction>) => (action: AnyAction) => {
// Call the next dispatch method in the middleware chain.
Expand All @@ -125,5 +147,19 @@ export function createSagaMiddleware(saga: Saga): Middleware {
};
}

middleware.start = () => {
runSaga();
};

middleware.stop = () => {
running = false;

// In order to actually stop the iterator instances, we need to force the loop to run one last time
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After setting running to false, the following actions won't have any effect, right? So why do we need to enqueue another action?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @peluja1012 - correct, this action will have no effect other than to force the loop to exit. The generator loop is paused awaiting the resolution of the current pending Promise. So when this action here is queued, that promise will resolve and the loop will exit out..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@peluja1012 - Rob had suggested perhaps using a second Promise to control the "stop action" and use Promise.race inside of the while (running) loop. Its a another approach and if that seems clearer to all, I can implement it that way.

enqueue({
action: { type: '' },
state: {} as GlobalState,
});
};

return middleware;
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ describe('endpoint list saga', () => {
let fakeHttpServices: jest.Mocked<HttpSetup>;
let store: Store<EndpointListState>;
let dispatch: Dispatch<EndpointListAction>;
let stopSagas: () => void;

// TODO: consolidate the below ++ helpers in `index.test.ts` into a `test_helpers.ts`??
const generateEndpoint = (): EndpointData => {
Expand Down Expand Up @@ -89,13 +90,19 @@ describe('endpoint list saga', () => {
beforeEach(() => {
fakeCoreStart = coreMock.createStart({ basePath: '/mock' });
fakeHttpServices = fakeCoreStart.http as jest.Mocked<HttpSetup>;
store = createStore(
endpointListReducer,
applyMiddleware(createSagaMiddleware(endpointListSagaFactory()))
);

const sagaMiddleware = createSagaMiddleware(endpointListSagaFactory());
store = createStore(endpointListReducer, applyMiddleware(sagaMiddleware));

sagaMiddleware.start();
stopSagas = sagaMiddleware.stop;
dispatch = store.dispatch;
});

afterEach(() => {
stopSagas();
});

test('it handles `userEnteredEndpointListPage`', async () => {
const apiResponse = getEndpointListApiResponse();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { createStore, compose, applyMiddleware } from 'redux';
import { createStore, compose, applyMiddleware, Store } from 'redux';
import { CoreStart } from 'kibana/public';
import { appSagaFactory } from './saga';
import { appReducer } from './reducer';
Expand All @@ -15,10 +15,13 @@ const composeWithReduxDevTools = (window as any).__REDUX_DEVTOOLS_EXTENSION_COMP
? (window as any).__REDUX_DEVTOOLS_EXTENSION_COMPOSE__({ name: 'EndpointApp' })
: compose;

export const appStoreFactory = (coreStart: CoreStart) => {
export const appStoreFactory = (coreStart: CoreStart): [Store, () => void] => {
const sagaReduxMiddleware = appSagaFactory(coreStart);
const store = createStore(
appReducer,
composeWithReduxDevTools(applyMiddleware(appSagaFactory(coreStart)))
composeWithReduxDevTools(applyMiddleware(sagaReduxMiddleware))
);
return store;

sagaReduxMiddleware.start();
return [store, sagaReduxMiddleware.stop];
};