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

Commit

Permalink
KIALI-1597 Don't try to update state on umounted components
Browse files Browse the repository at this point in the history 
Callback of some async data fetches were trying to update the state of
already umounted components. This was causing some errors to appear in
the browser console.

Currently, errors aren't causing any problems, but by stopping further
processing when component umounts this may prevent future problems.

This is also fixing some issues when sorting lists.
  • Loading branch information
@israel-hdez
israel-hdez committed Oct 5, 2018
1 parent 10a54ff commit 80e0e62
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 80e0e62

Please sign in to comment.