Skip to content
This repository has been archived by the owner on Apr 11, 2023. It is now read-only.

Commit

Permalink
Merge pull request #727 from israel-hdez/kiali-1597
Browse files Browse the repository at this point in the history 
KIALI-1597 Don't try to update state on umounted components
  • Loading branch information
@israel-hdez
israel-hdez authored Oct 5, 2018
2 parents 10a54ff + 80e0e62 commit 8edd590
Show file tree
Hide file tree
Showing 9 changed files with 512 additions and 126 deletions.
24 changes: 21 additions & 3 deletions src/components/Filters/StatefulFilters.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import * as React from 'react';
import { Filter, FormControl, Toolbar } from 'patternfly-react';
import { ActiveFilter, FILTER_ACTION_UPDATE, FilterType, FilterValue } from '../../types/Filters';
import { ListPage } from '../ListPage/ListPage';
import { CancelablePromise, makeCancelablePromise } from '../../utils/Common';

export interface StatefulFiltersProps {
onFilterChange: () => void;
Expand Down Expand Up @@ -33,6 +34,8 @@ export namespace FilterSelected {
}

export class StatefulFilters extends React.Component<StatefulFiltersProps, StatefulFiltersState> {
private filterTypePromise?: CancelablePromise<FilterType[]>;

constructor(props: StatefulFiltersProps) {
super(props);

Expand Down Expand Up @@ -73,9 +76,17 @@ export class StatefulFilters extends React.Component<StatefulFiltersProps, State
}
});

Promise.all(promises).then(types => {
this.setState({ filterTypes: types });
});
this.filterTypePromise = makeCancelablePromise(Promise.all(promises));
this.filterTypePromise.promise
.then(types => {
this.setState({ filterTypes: types });
this.filterTypePromise = undefined;
})
.catch(err => {
if (!err.isCancelable) {
console.debug(err);
}
});
}

componentDidUpdate(prevProps: StatefulFiltersProps, prevState: StatefulFiltersState, snapshot: any) {
Expand All @@ -84,6 +95,13 @@ export class StatefulFilters extends React.Component<StatefulFiltersProps, State
}
}

componentWillUnmount() {
if (this.filterTypePromise) {
this.filterTypePromise.cancel();
this.filterTypePromise = undefined;
}
}

updateActiveFilters(activeFilters: ActiveFilter[]) {
const cleanFilters = this.props.pageHooks.setFiltersToURL(this.state.filterTypes, activeFilters);
FilterSelected.setSelected(cleanFilters);
Expand Down
134 changes: 107 additions & 27 deletions src/pages/AppList/AppListComponent.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@ import * as React from 'react';
import * as API from '../../services/Api';
import { authentication } from '../../utils/Authentication';
import Namespace from '../../types/Namespace';
import { AppListItem } from '../../types/AppList';
import { AppList, AppListItem } from '../../types/AppList';
import { AppListFilters } from './FiltersAndSorts';
import { AppListClass } from './AppListClass';
import { FilterSelected, StatefulFilters } from '../../components/Filters/StatefulFilters';
import { Button, Icon, ListView, Paginator, Sort, ToolbarRightContent } from 'patternfly-react';
import { ActiveFilter } from '../../types/Filters';
import { removeDuplicatesArray } from '../../utils/Common';
import { CancelablePromise, makeCancelablePromise, removeDuplicatesArray } from '../../utils/Common';
import RateIntervalToolbarItem from '../ServiceList/RateIntervalToolbarItem';
import { ListPage } from '../../components/ListPage/ListPage';
import { SortField } from '../../types/SortFilters';
Expand All @@ -24,6 +24,10 @@ interface AppListComponentProps extends ListComponent.Props<AppListItem> {
}

class AppListComponent extends ListComponent.Component<AppListComponentProps, AppListComponentState, AppListItem> {
private nsPromise?: CancelablePromise<API.Response<Namespace[]>>;
private appPromise?: CancelablePromise<API.Response<AppList>[]>;
private sortPromise?: CancelablePromise<AppListItem[]>;

constructor(props: AppListComponentProps) {
super(props);
this.state = {
Expand Down Expand Up @@ -51,6 +55,10 @@ class AppListComponent extends ListComponent.Component<AppListComponentProps, Ap
}
}

componentWillUnmount() {
this.cancelAsyncs();
}

paramsAreSynced(prevProps: AppListComponentProps) {
return (
prevProps.pagination.page === this.props.pagination.page &&
Expand All @@ -66,11 +74,39 @@ class AppListComponent extends ListComponent.Component<AppListComponentProps, Ap
this.setState({ rateInterval: key });
};

sortItemList(apps: AppListItem[], sortField: SortField<AppListItem>, isAscending: boolean) {
return AppListFilters.sortAppsItems(apps, sortField, isAscending);
sortItemList(apps: AppListItem[], sortField: SortField<AppListItem>, isAscending: boolean): Promise<AppListItem[]> {
let lastSort: Promise<AppListItem[]>;
const sorter = unsorted => {
this.sortPromise = makeCancelablePromise(AppListFilters.sortAppsItems(unsorted, sortField, isAscending));
this.sortPromise.promise
.then(() => {
this.sortPromise = undefined;
})
.catch(() => {
this.sortPromise = undefined;
});
return this.sortPromise.promise;
};

if (!this.sortPromise) {
// If there is no "sortPromise" set, take the received (unsorted) list of apps to sort
// them and update the UI with the sorted list.
lastSort = sorter(apps);
} else {
// If there is a "sortPromise", there may be an ongoing fetch/refresh. So, the received <apps> list argument
// shoudn't be used as it may represent the "old" data before the refresh. Instead, append a callback to the
// "sortPromise" to re-sort once the data is fetched. This ensures that the list will display the new data with
// the right sorting.
// (See other comments in the fetchApps method)
lastSort = this.sortPromise.promise.then(sorter);
}

return lastSort;
}

updateListItems(resetPagination?: boolean) {
this.cancelAsyncs();

const activeFilters: ActiveFilter[] = FilterSelected.getSelected();
let namespacesSelected: string[] = activeFilters
.filter(activeFilter => activeFilter.category === 'Namespace')
Expand All @@ -80,7 +116,8 @@ class AppListComponent extends ListComponent.Component<AppListComponentProps, Ap
namespacesSelected = removeDuplicatesArray(namespacesSelected);

if (namespacesSelected.length === 0) {
API.getNamespaces(authentication())
this.nsPromise = makeCancelablePromise(API.getNamespaces(authentication()));
this.nsPromise.promise
.then(namespacesResponse => {
const namespaces: Namespace[] = namespacesResponse['data'];
this.fetchApps(
Expand All @@ -89,39 +126,67 @@ class AppListComponent extends ListComponent.Component<AppListComponentProps, Ap
this.props.rateInterval,
resetPagination
);
this.nsPromise = undefined;
})
.catch(namespacesError => this.handleAxiosError('Could not fetch namespace list.', namespacesError));
.catch(namespacesError => {
if (!namespacesError.isCanceled) {
this.handleAxiosError('Could not fetch namespace list.', namespacesError);
}
});
} else {
this.fetchApps(namespacesSelected, activeFilters, this.props.rateInterval, resetPagination);
}
}

fetchApps(namespaces: string[], filters: ActiveFilter[], rateInterval: number, resetPagination?: boolean) {
const appsPromises = namespaces.map(namespace => API.getApps(authentication(), namespace));
Promise.all(appsPromises).then(responses => {
const currentPage = resetPagination ? 1 : this.state.pagination.page;
this.appPromise = makeCancelablePromise(Promise.all(appsPromises));
this.appPromise.promise
.then(responses => {
const currentPage = resetPagination ? 1 : this.state.pagination.page;

let appListItems: AppListItem[] = [];
responses.forEach(response => {
AppListFilters.filterBy(response.data, filters);
appListItems = appListItems.concat(AppListClass.getAppItems(response.data, rateInterval));
});

AppListFilters.sortAppsItems(appListItems, this.state.currentSortField, this.state.isSortAscending).then(
sorted => {
this.setState(prevState => {
return {
listItems: sorted,
pagination: {
page: currentPage,
perPage: prevState.pagination.perPage,
perPageOptions: ListPage.perPageOptions
}
};
let appListItems: AppListItem[] = [];
responses.forEach(response => {
AppListFilters.filterBy(response.data, filters);
appListItems = appListItems.concat(AppListClass.getAppItems(response.data, rateInterval));
});
if (this.sortPromise) {
this.sortPromise.cancel();
}
// Promises for sorting are needed, because the user may have the list sorted using health/error rates
// and these data can be fetched only after the list is retrieved. If the user is sorting using these
// criteria, the update of the list is deferred after sorting is possible. This way, it's avoided the
// illusion of double-fetch or flickering list.
this.sortPromise = makeCancelablePromise(
AppListFilters.sortAppsItems(appListItems, this.state.currentSortField, this.state.isSortAscending)
);
this.sortPromise.promise
.then(sorted => {
this.setState(prevState => {
return {
listItems: sorted,
pagination: {
page: currentPage,
perPage: prevState.pagination.perPage,
perPageOptions: ListPage.perPageOptions
}
};
});
this.sortPromise = undefined;
})
.catch(err => {
if (!err.isCanceled) {
console.debug(err);
}
this.sortPromise = undefined;
});
this.appPromise = undefined;
})
.catch(err => {
if (!err.isCanceled) {
console.debug(err);
}
);
});
});
}

render() {
Expand Down Expand Up @@ -174,6 +239,21 @@ class AppListComponent extends ListComponent.Component<AppListComponentProps, Ap
</>
);
}

private cancelAsyncs = () => {
if (this.nsPromise) {
this.nsPromise.cancel();
this.nsPromise = undefined;
}
if (this.appPromise) {
this.appPromise.cancel();
this.appPromise = undefined;
}
if (this.sortPromise) {
this.sortPromise.cancel();
this.sortPromise = undefined;
}
};
}

export default AppListComponent;
21 changes: 20 additions & 1 deletion src/pages/AppList/ItemDescription.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { AppHealth } from '../../types/Health';
import { DisplayMode, HealthIndicator } from '../../components/Health/HealthIndicator';
import AppErrorRate from './AppErrorRate';
import { AppListItem } from '../../types/AppList';
import { CancelablePromise, makeCancelablePromise } from '../../utils/Common';

interface Props {
item: AppListItem;
Expand All @@ -12,6 +13,8 @@ interface State {
}

export default class ItemDescription extends React.PureComponent<Props, State> {
private healthPromise?: CancelablePromise<AppHealth>;

constructor(props: Props) {
super(props);
this.state = { health: undefined };
Expand All @@ -27,8 +30,24 @@ export default class ItemDescription extends React.PureComponent<Props, State> {
}
}

componentWillUnmount() {
if (this.healthPromise) {
this.healthPromise.cancel();
this.healthPromise = undefined;
}
}

onItemChanged(item: AppListItem) {
item.healthPromise.then(h => this.setState({ health: h })).catch(err => this.setState({ health: undefined }));
if (this.healthPromise) {
this.healthPromise.cancel();
}

this.healthPromise = makeCancelablePromise(item.healthPromise);
this.healthPromise.promise.then(h => this.setState({ health: h })).catch(err => {
if (!err.isCanceled) {
this.setState({ health: undefined });
}
});
}

render() {
Expand Down
Loading

0 comments on commit 8edd590

Please sign in to comment.