Skip to content

[Console] Fix leaky mappings subscription #45646

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.

Sign up for GitHub

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

Merged
jloleysens merged 3 commits into from
Sep 16, 2019
Merged

[Console] Fix leaky mappings subscription #45646

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4bc1901
Fix mappings subscription (still needs further refactoring)
jloleysens Sep 13, 2019
fd32bfc
More defensive removeChild behaviour on unmount
jloleysens Sep 16, 2019
5753c53
Merge branch 'master' into fix_leaky_polling
elasticmachine Sep 16, 2019
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
5 changes: 4 additions & 1 deletion ...console/np_ready/public/application/containers/editor/legacy/use_ui_ace_keyboard_mode.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,10 @@ export function useUIAceKeyboardMode(aceTextAreaElement: HTMLTextAreaElement | n
if (aceTextAreaElement) {
document.removeEventListener('keydown', documentKeyDownListener);
aceTextAreaElement.removeEventListener('keydown', aceKeydownListener);
document.removeChild(overlayMountNode.current!);
Copy link
Copy Markdown
Contributor

@cjcenizal cjcenizal Sep 13, 2019

Choose a reason for hiding this comment

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

Why is this being removed?

Copy link
Copy Markdown
Contributor Author

@jloleysens jloleysens Sep 16, 2019

Choose a reason for hiding this comment

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

I was running into the first of these two issues: https://developer.mozilla.org/en-US/docs/Web/API/Node/removeChild#Errors_thrown

Which indicated that cleanup for this DOM node is happening with the react component unmounting.

Copy link
Copy Markdown
Contributor Author

@jloleysens jloleysens Sep 16, 2019

Choose a reason for hiding this comment

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

Update: I re-implemented this in a more robust way.

const textAreaContainer = aceTextAreaElement.parentElement;
if (textAreaContainer && textAreaContainer.contains(overlayMountNode.current!)) {
textAreaContainer.removeChild(overlayMountNode.current!);
}
}
};
}, [aceTextAreaElement]);
Expand Down
9 changes: 7 additions & 2 deletions src/legacy/core_plugins/console/np_ready/public/application/containers/main/main.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,13 +176,17 @@ export function Main() {

useEffect(() => {
let resizerSubscriptions: Array<() => void> = [];
const subscription = editorsReady$.subscribe(([input, output]) => {
const readySubscription = editorsReady$.subscribe(([input, output]) => {
settings.registerOutput(output.editor);
settings.registerInput(input.editor);
history.setEditor(input.editor);

init(input.editor, output.editor, history);

// This may kick of a polling mechanic, needs to be refactored to more
// standard subscription pattern.
mappings.retrieveAutoCompleteInfo();

resizerSubscriptions = resizerSubscriptions.concat([
subscribeResizeChecker(ResizeChecker, containerRef.current!, input.editor, output.editor),
subscribeResizeChecker(ResizeChecker, input.element, input.editor),
Expand All @@ -196,7 +200,8 @@ export function Main() {

return () => {
resizerSubscriptions.map(done => done());
subscription.unsubscribe();
readySubscription.unsubscribe();
mappings.clearSubscriptions();
};
}, []);

Expand Down
13 changes: 11 additions & 2 deletions src/legacy/core_plugins/console/np_ready/public/legacy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import 'ui/capabilities/route_setup';
/* eslint-enable @kbn/eslint/no-restricted-paths */

import template from '../../public/quarantined/index.html';
import { App } from '../../../../../core/public';
import { App, AppUnmount } from '../../../../../core/public';

export interface XPluginSet {
__LEGACY: {
Expand Down Expand Up @@ -64,12 +64,14 @@ uiRoutes.when('/dev_tools/console', {
throw new Error(message);
}

let unmount: AppUnmount | Promise<AppUnmount>;
Copy link
Copy Markdown
Contributor

@cjcenizal cjcenizal Sep 13, 2019

Choose a reason for hiding this comment

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

Could you clarify for me the scenario in which unmount would be assigned a promise value?

Copy link
Copy Markdown
Contributor Author

@jloleysens jloleysens Sep 16, 2019

Choose a reason for hiding this comment

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

This was done to satisfy the type constraints introduced by core: https://github.com/elastic/kibana/blob/master/src/core/public/application/types.ts#L82


const mockedSetupCore = {
...npSetup.core,
application: {
register(app: App): void {
try {
app.mount(anyObject, { element: targetElement, appBasePath: '' });
unmount = app.mount(anyObject, { element: targetElement, appBasePath: '' });
} catch (e) {
npSetup.core.fatalErrors.add(e);
}
Expand All @@ -87,6 +89,13 @@ uiRoutes.when('/dev_tools/console', {
},
});
pluginInstance.start(npStart.core);

$scope.$on('$destroy', async () => {
if (unmount) {
const fn = await unmount;
fn();
}
});
};
},
template,
Expand Down
2 changes: 0 additions & 2 deletions src/legacy/core_plugins/console/public/quarantined/src/app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
*/

const $ = require('jquery');
const mappings = require('./mappings');

const DEFAULT_INPUT_VALUE = `GET _search
{
Expand Down Expand Up @@ -92,5 +91,4 @@ export default function init(input, output, history, sourceLocation = 'stored')

setupAutosave();
loadSavedState();
mappings.retrieveAutoCompleteInfo();
}
9 changes: 8 additions & 1 deletion src/legacy/core_plugins/console/public/quarantined/src/mappings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,10 +290,16 @@ function retrieveSettings(settingsKey, settingsToRetrieve) {
// unchanged alone (both selected and unselected).
// 3. Poll: Use saved. Fetch selected. Ignore unselected.

function retrieveAutoCompleteInfo(settingsToRetrieve = legacyBackDoorToSettings().getAutocomplete()) {

function clearSubscriptions() {
if (pollTimeoutId) {
clearTimeout(pollTimeoutId);
}
}


function retrieveAutoCompleteInfo(settingsToRetrieve = legacyBackDoorToSettings().getAutocomplete()) {
clearSubscriptions();

const mappingPromise = retrieveSettings('fields', settingsToRetrieve);
const aliasesPromise = retrieveSettings('indices', settingsToRetrieve);
Expand Down Expand Up @@ -347,4 +353,5 @@ export default {
expandAliases,
clear,
retrieveAutoCompleteInfo,
clearSubscriptions
};