[NP] Add lifecycle timeout (elastic#54129) (elastic#54346)

* add promise timeout decorator * crash Kibana if lifecycle takes > 30sec Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
tsullivan · Jan 9, 2020 · 1697f21 · 1697f21
1 parent 40ca870
commit 1697f21
Show file tree

Hide file tree

Showing 7 changed files with 237 additions and 20 deletions.
diff --git a/src/core/public/plugins/plugins_service.test.ts b/src/core/public/plugins/plugins_service.test.ts
@@ -279,6 +279,37 @@ describe('PluginsService', () => {
       expect((contracts.get('pluginA')! as any).setupValue).toEqual(1);
       expect((contracts.get('pluginB')! as any).pluginAPlusB).toEqual(2);
     });
+
+    describe('timeout', () => {
+      const flushPromises = () => new Promise(resolve => setImmediate(resolve));
+      beforeAll(() => {
+        jest.useFakeTimers();
+      });
+      afterAll(() => {
+        jest.useRealTimers();
+      });
+
+      it('throws timeout error if "setup" was not completed in 30 sec.', async () => {
+        mockPluginInitializers.set(
+          'pluginA',
+          jest.fn(() => ({
+            setup: jest.fn(() => new Promise(i => i)),
+            start: jest.fn(() => ({ value: 1 })),
+            stop: jest.fn(),
+          }))
+        );
+        const pluginsService = new PluginsService(mockCoreContext, plugins);
+        const promise = pluginsService.setup(mockSetupDeps);
+
+        jest.runAllTimers(); // load plugin bundles
+        await flushPromises();
+        jest.runAllTimers(); // setup plugins
+
+        await expect(promise).rejects.toMatchInlineSnapshot(
+          `[Error: Setup lifecycle of "pluginA" plugin wasn't completed in 30sec. Consider disabling the plugin and re-start.]`
+        );
+      });
+    });
   });
 
   describe('#start()', () => {
@@ -331,6 +362,34 @@ describe('PluginsService', () => {
       expect((contracts.get('pluginA')! as any).startValue).toEqual(2);
       expect((contracts.get('pluginB')! as any).pluginAPlusB).toEqual(3);
     });
+    describe('timeout', () => {
+      beforeAll(() => {
+        jest.useFakeTimers();
+      });
+      afterAll(() => {
+        jest.useRealTimers();
+      });
+
+      it('throws timeout error if "start" was not completed in 30 sec.', async () => {
+        mockPluginInitializers.set(
+          'pluginA',
+          jest.fn(() => ({
+            setup: jest.fn(() => ({ value: 1 })),
+            start: jest.fn(() => new Promise(i => i)),
+            stop: jest.fn(),
+          }))
+        );
+        const pluginsService = new PluginsService(mockCoreContext, plugins);
+        await pluginsService.setup(mockSetupDeps);
+
+        const promise = pluginsService.start(mockStartDeps);
+        jest.runAllTimers();
+
+        await expect(promise).rejects.toMatchInlineSnapshot(
+          `[Error: Start lifecycle of "pluginA" plugin wasn't completed in 30sec. Consider disabling the plugin and re-start.]`
+        );
+      });
+    });
   });
 
   describe('#stop()', () => {

diff --git a/src/core/public/plugins/plugins_service.ts b/src/core/public/plugins/plugins_service.ts
@@ -28,7 +28,9 @@ import {
 } from './plugin_context';
 import { InternalCoreSetup, InternalCoreStart } from '../core_system';
 import { InjectedPluginMetadata } from '../injected_metadata';
+import { withTimeout } from '../../utils';
 
+const Sec = 1000;
 /** @internal */
 export type PluginsServiceSetupDeps = InternalCoreSetup;
 /** @internal */
@@ -110,13 +112,15 @@ export class PluginsService implements CoreService<PluginsServiceSetup, PluginsS
         {} as Record<PluginName, unknown>
       );
 
-      contracts.set(
-        pluginName,
-        await plugin.setup(
+      const contract = await withTimeout({
+        promise: plugin.setup(
           createPluginSetupContext(this.coreContext, deps, plugin),
           pluginDepContracts
-        )
-      );
+        ),
+        timeout: 30 * Sec,
+        errorMessage: `Setup lifecycle of "${pluginName}" plugin wasn't completed in 30sec. Consider disabling the plugin and re-start.`,
+      });
+      contracts.set(pluginName, contract);
 
       this.satupPlugins.push(pluginName);
     }
@@ -142,13 +146,15 @@ export class PluginsService implements CoreService<PluginsServiceSetup, PluginsS
         {} as Record<PluginName, unknown>
       );
 
-      contracts.set(
-        pluginName,
-        await plugin.start(
+      const contract = await withTimeout({
+        promise: plugin.start(
           createPluginStartContext(this.coreContext, deps, plugin),
           pluginDepContracts
-        )
-      );
+        ),
+        timeout: 30 * Sec,
+        errorMessage: `Start lifecycle of "${pluginName}" plugin wasn't completed in 30sec. Consider disabling the plugin and re-start.`,
+      });
+      contracts.set(pluginName, contract);
     }
 
     // Expose start contracts

diff --git a/src/core/server/plugins/plugins_system.test.ts b/src/core/server/plugins/plugins_system.test.ts
@@ -414,3 +414,51 @@ test('`startPlugins` only starts plugins that were setup', async () => {
     ]
   `);
 });
+
+describe('setup', () => {
+  beforeAll(() => {
+    jest.useFakeTimers();
+  });
+  afterAll(() => {
+    jest.useRealTimers();
+  });
+  it('throws timeout error if "setup" was not completed in 30 sec.', async () => {
+    const plugin: PluginWrapper = createPlugin('timeout-setup');
+    jest.spyOn(plugin, 'setup').mockImplementation(() => new Promise(i => i));
+    pluginsSystem.addPlugin(plugin);
+    mockCreatePluginSetupContext.mockImplementation(() => ({}));
+
+    const promise = pluginsSystem.setupPlugins(setupDeps);
+    jest.runAllTimers();
+
+    await expect(promise).rejects.toMatchInlineSnapshot(
+      `[Error: Setup lifecycle of "timeout-setup" plugin wasn't completed in 30sec. Consider disabling the plugin and re-start.]`
+    );
+  });
+});
+
+describe('start', () => {
+  beforeAll(() => {
+    jest.useFakeTimers();
+  });
+  afterAll(() => {
+    jest.useRealTimers();
+  });
+  it('throws timeout error if "start" was not completed in 30 sec.', async () => {
+    const plugin: PluginWrapper = createPlugin('timeout-start');
+    jest.spyOn(plugin, 'setup').mockResolvedValue({});
+    jest.spyOn(plugin, 'start').mockImplementation(() => new Promise(i => i));
+
+    pluginsSystem.addPlugin(plugin);
+    mockCreatePluginSetupContext.mockImplementation(() => ({}));
+    mockCreatePluginStartContext.mockImplementation(() => ({}));
+
+    await pluginsSystem.setupPlugins(setupDeps);
+    const promise = pluginsSystem.startPlugins(startDeps);
+    jest.runAllTimers();
+
+    await expect(promise).rejects.toMatchInlineSnapshot(
+      `[Error: Start lifecycle of "timeout-start" plugin wasn't completed in 30sec. Consider disabling the plugin and re-start.]`
+    );
+  });
+});
diff --git a/src/core/server/plugins/plugins_system.ts b/src/core/server/plugins/plugins_system.ts
@@ -23,7 +23,9 @@ import { PluginWrapper } from './plugin';
 import { DiscoveredPlugin, PluginName, PluginOpaqueId } from './types';
 import { createPluginSetupContext, createPluginStartContext } from './plugin_context';
 import { PluginsServiceSetupDeps, PluginsServiceStartDeps } from './plugins_service';
+import { withTimeout } from '../../utils';
 
+const Sec = 1000;
 /** @internal */
 export class PluginsSystem {
   private readonly plugins = new Map<PluginName, PluginWrapper>();
@@ -85,14 +87,16 @@ export class PluginsSystem {
         return depContracts;
       }, {} as Record<PluginName, unknown>);
 
-      contracts.set(
-        pluginName,
-        await plugin.setup(
+      const contract = await withTimeout({
+        promise: plugin.setup(
           createPluginSetupContext(this.coreContext, deps, plugin),
           pluginDepContracts
-        )
-      );
+        ),
+        timeout: 30 * Sec,
+        errorMessage: `Setup lifecycle of "${pluginName}" plugin wasn't completed in 30sec. Consider disabling the plugin and re-start.`,
+      });
 
+      contracts.set(pluginName, contract);
       this.satupPlugins.push(pluginName);
     }
 
@@ -121,13 +125,16 @@ export class PluginsSystem {
         return depContracts;
       }, {} as Record<PluginName, unknown>);
 
-      contracts.set(
-        pluginName,
-        await plugin.start(
+      const contract = await withTimeout({
+        promise: plugin.start(
           createPluginStartContext(this.coreContext, deps, plugin),
           pluginDepContracts
-        )
-      );
+        ),
+        timeout: 30 * Sec,
+        errorMessage: `Start lifecycle of "${pluginName}" plugin wasn't completed in 30sec. Consider disabling the plugin and re-start.`,
+      });
+
+      contracts.set(pluginName, contract);
     }
 
     return contracts;

diff --git a/src/core/utils/index.ts b/src/core/utils/index.ts
@@ -24,5 +24,6 @@ export * from './get';
 export * from './map_to_object';
 export * from './merge';
 export * from './pick';
+export * from './promise';
 export * from './url';
 export * from './unset';
diff --git a/src/core/utils/promise.test.ts b/src/core/utils/promise.test.ts
@@ -0,0 +1,63 @@
+/*
+ * Licensed to Elasticsearch B.V. under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch B.V. licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { withTimeout } from './promise';
+
+const delay = (ms: number, resolveValue?: any) =>
+  new Promise(resolve => setTimeout(resolve, ms, resolveValue));
+
+describe('withTimeout', () => {
+  it('resolves with a promise value if resolved in given timeout', async () => {
+    await expect(
+      withTimeout({
+        promise: delay(10, 'value'),
+        timeout: 200,
+        errorMessage: 'error-message',
+      })
+    ).resolves.toBe('value');
+  });
+
+  it('rejects with errorMessage if not resolved in given time', async () => {
+    await expect(
+      withTimeout({
+        promise: delay(200, 'value'),
+        timeout: 10,
+        errorMessage: 'error-message',
+      })
+    ).rejects.toMatchInlineSnapshot(`[Error: error-message]`);
+
+    await expect(
+      withTimeout({
+        promise: new Promise(i => i),
+        timeout: 10,
+        errorMessage: 'error-message',
+      })
+    ).rejects.toMatchInlineSnapshot(`[Error: error-message]`);
+  });
+
+  it('does not swallow promise error', async () => {
+    await expect(
+      withTimeout({
+        promise: Promise.reject(new Error('from-promise')),
+        timeout: 10,
+        errorMessage: 'error-message',
+      })
+    ).rejects.toMatchInlineSnapshot(`[Error: from-promise]`);
+  });
+});
diff --git a/src/core/utils/promise.ts b/src/core/utils/promise.ts
@@ -0,0 +1,33 @@
+/*
+ * Licensed to Elasticsearch B.V. under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch B.V. licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+export function withTimeout<T>({
+  promise,
+  timeout,
+  errorMessage,
+}: {
+  promise: Promise<T>;
+  timeout: number;
+  errorMessage: string;
+}) {
+  return Promise.race([
+    promise,
+    new Promise((resolve, reject) => setTimeout(() => reject(new Error(errorMessage)), timeout)),
+  ]) as Promise<T>;
+}