From 80e0e62c7dd68c5ff61ccb176f68b00585f6bae9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Edgar=20Hern=C3=A1ndez?= Date: Mon, 1 Oct 2018 17:07:50 -0500 Subject: [PATCH] KIALI-1597 Don't try to update state on umounted components 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. --- src/components/Filters/StatefulFilters.tsx | 24 ++- src/pages/AppList/AppListComponent.tsx | 134 ++++++++++++---- src/pages/AppList/ItemDescription.tsx | 21 ++- .../IstioConfigListComponent.tsx | 145 +++++++++++++----- src/pages/ServiceList/ItemDescription.tsx | 21 ++- .../ServiceList/ServiceListComponent.tsx | 130 +++++++++++++--- .../__tests__/ItemDescription.test.tsx | 5 +- src/pages/WorkloadList/ItemDescription.tsx | 21 ++- .../WorkloadList/WorkloadListComponent.tsx | 137 +++++++++++++---- 9 files changed, 512 insertions(+), 126 deletions(-) diff --git a/src/components/Filters/StatefulFilters.tsx b/src/components/Filters/StatefulFilters.tsx index 7df9e5a5fd..ad84cf1feb 100644 --- a/src/components/Filters/StatefulFilters.tsx +++ b/src/components/Filters/StatefulFilters.tsx @@ -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; @@ -33,6 +34,8 @@ export namespace FilterSelected { } export class StatefulFilters extends React.Component { + private filterTypePromise?: CancelablePromise; + constructor(props: StatefulFiltersProps) { super(props); @@ -73,9 +76,17 @@ export class StatefulFilters extends React.Component { - 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) { @@ -84,6 +95,13 @@ export class StatefulFilters extends React.Component { } class AppListComponent extends ListComponent.Component { + private nsPromise?: CancelablePromise>; + private appPromise?: CancelablePromise[]>; + private sortPromise?: CancelablePromise; + constructor(props: AppListComponentProps) { super(props); this.state = { @@ -51,6 +55,10 @@ class AppListComponent extends ListComponent.Component, isAscending: boolean) { - return AppListFilters.sortAppsItems(apps, sortField, isAscending); + sortItemList(apps: AppListItem[], sortField: SortField, isAscending: boolean): Promise { + let lastSort: Promise; + 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 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') @@ -80,7 +116,8 @@ class AppListComponent extends ListComponent.Component { const namespaces: Namespace[] = namespacesResponse['data']; this.fetchApps( @@ -89,8 +126,13 @@ class AppListComponent extends ListComponent.Component 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); } @@ -98,30 +140,53 @@ class AppListComponent extends ListComponent.Component 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() { @@ -174,6 +239,21 @@ class AppListComponent extends ListComponent.Component ); } + + 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; diff --git a/src/pages/AppList/ItemDescription.tsx b/src/pages/AppList/ItemDescription.tsx index f88bb336f5..052a68c800 100644 --- a/src/pages/AppList/ItemDescription.tsx +++ b/src/pages/AppList/ItemDescription.tsx @@ -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; @@ -12,6 +13,8 @@ interface State { } export default class ItemDescription extends React.PureComponent { + private healthPromise?: CancelablePromise; + constructor(props: Props) { super(props); this.state = { health: undefined }; @@ -27,8 +30,24 @@ export default class ItemDescription extends React.PureComponent { } } + 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() { diff --git a/src/pages/IstioConfigList/IstioConfigListComponent.tsx b/src/pages/IstioConfigList/IstioConfigListComponent.tsx index 8d437eda91..f53f5dbbf5 100644 --- a/src/pages/IstioConfigList/IstioConfigListComponent.tsx +++ b/src/pages/IstioConfigList/IstioConfigListComponent.tsx @@ -25,7 +25,7 @@ import { PfColors } from '../../components/Pf/PfColors'; import { authentication } from '../../utils/Authentication'; import { NamespaceValidations } from '../../types/IstioObjects'; import { ConfigIndicator } from '../../components/ConfigValidation/ConfigIndicator'; -import { removeDuplicatesArray } from '../../utils/Common'; +import { CancelablePromise, makeCancelablePromise, removeDuplicatesArray } from '../../utils/Common'; import { ListPage } from '../../components/ListPage/ListPage'; import { IstioConfigListFilters } from './FiltersAndSorts'; import { ListComponent } from '../../components/ListPage/ListComponent'; @@ -39,6 +39,9 @@ class IstioConfigListComponent extends ListComponent.Component< IstioConfigListComponentState, IstioConfigItem > { + private nsPromise?: CancelablePromise>; + private configsPromise?: CancelablePromise; + constructor(props: IstioConfigListComponentProps) { super(props); this.state = { @@ -69,6 +72,10 @@ class IstioConfigListComponent extends ListComponent.Component< } } + componentWillUnmount() { + this.cancelAsyncs(); + } + paramsAreSynced(prevProps: IstioConfigListComponentProps) { return ( prevProps.pagination.page === this.props.pagination.page && @@ -83,6 +90,8 @@ class IstioConfigListComponent extends ListComponent.Component< } updateListItems(resetPagination?: boolean) { + this.cancelAsyncs(); + const activeFilters: ActiveFilter[] = FilterSelected.getSelected(); let namespacesSelected: string[] = activeFilters .filter(activeFilter => activeFilter.category === 'Namespace') @@ -104,7 +113,8 @@ class IstioConfigListComponent extends ListComponent.Component< configValidationFilters = removeDuplicatesArray(configValidationFilters); if (namespacesSelected.length === 0) { - API.getNamespaces(authentication()) + this.nsPromise = makeCancelablePromise(API.getNamespaces(authentication())); + this.nsPromise.promise .then(namespacesResponse => { const namespaces: Namespace[] = namespacesResponse['data']; this.fetchIstioConfig( @@ -114,8 +124,13 @@ class IstioConfigListComponent extends ListComponent.Component< configValidationFilters, 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.fetchIstioConfig( namespacesSelected, @@ -147,53 +162,94 @@ class IstioConfigListComponent extends ListComponent.Component< configValidationFilters: string[], resetPagination?: boolean ) { + // Retrieve the istio config list/items const istioConfigPromises = namespaces.map(namespace => API.getIstioConfig(authentication(), namespace, istioTypeFilters) ); - const validationPromises = namespaces.map(namespace => API.getNamespaceValidations(authentication(), namespace)); + // There is the advantage that there is no need to wait until the istio config list is fetched in order + // to fetch validations. So, lets retrieve validations to save time. + const validationPromises = Promise.all( + namespaces.map(namespace => API.getNamespaceValidations(authentication(), namespace)) + ).then(responses => { + let namespaceValidations: NamespaceValidations = {}; + responses.forEach(response => + Object.keys(response.data).forEach(namespace => (namespaceValidations[namespace] = response.data[namespace])) + ); + return namespaceValidations; + }); - Promise.all(istioConfigPromises) - .then(responses => { - const currentPage = resetPagination ? 1 : this.state.pagination.page; + // Once all istio configs are retrieved, apply filters and map the received data into a flattened list. + // "fetchPromise" is the one that will update the state to show the list. + let fetchPromise = Promise.all(istioConfigPromises).then(responses => { + let istioItems: IstioConfigItem[] = []; + responses.forEach(response => { + istioItems = istioItems.concat(toIstioItems(filterByName(response.data, istioNameFilters))); + }); - let istioItems: IstioConfigItem[] = []; - responses.forEach(response => { - istioItems = istioItems.concat(toIstioItems(filterByName(response.data, istioNameFilters))); + return istioItems; + }); + + if (configValidationFilters.length > 0 || this.state.currentSortField.id === 'configvalidation') { + // If user *is* filtering and/or sorting using "validations", we must wait until the validations are fetched in order + // to update/sort the view. This way, we avoid a flickering list and/or ending up with a wrong sorting. + // So, unify and . + fetchPromise = fetchPromise.then(items => { + return validationPromises.then(namespaceValidations => { + return filterByConfigValidation(this.updateValidation(items, namespaceValidations), configValidationFilters); }); + }); + } - IstioConfigListFilters.sortIstioItems(istioItems, this.state.currentSortField, this.state.isSortAscending).then( - sortedItems => { - this.setState(prevState => { - return { - listItems: sortedItems, - pagination: { - page: currentPage, - perPage: prevState.pagination.perPage, - perPageOptions: ListPage.perPageOptions - } - }; - }); - } - ); + // Update the view when data is fetched --- Make cancelable to avoid updating + // the state if the component umounts before data retrieval finishes. + this.configsPromise = makeCancelablePromise(fetchPromise); + this.configsPromise.promise + .then(istioItems => { + const currentPage = resetPagination ? 1 : this.state.pagination.page; - return Promise.all(validationPromises); - }) - .then(responses => { - let namespaceValidations: NamespaceValidations = {}; - responses.forEach(response => - Object.keys(response.data).forEach(namespace => (namespaceValidations[namespace] = response.data[namespace])) - ); - this.setState(prevState => { - return { - listItems: filterByConfigValidation( - this.updateValidation(prevState.listItems, namespaceValidations), - configValidationFilters - ) - }; + IstioConfigListFilters.sortIstioItems( + istioItems as IstioConfigItem[], + this.state.currentSortField, + this.state.isSortAscending + ).then(sorted => { + this.setState(prevState => { + return { + listItems: sorted, + pagination: { + page: currentPage, + perPage: prevState.pagination.perPage, + perPageOptions: ListPage.perPageOptions + } + }; + }); }); }) - .catch(istioError => this.handleAxiosError('Could not fetch Istio objects list.', istioError)); + .catch(istioError => { + if (!istioError.isCanceled) { + this.handleAxiosError('Could not fetch Istio objects list.', istioError); + } + }); + + if (configValidationFilters.length === 0 && this.state.currentSortField.id !== 'configvalidation') { + // If user *is not* filtering nor sorting using "validations", we can show the list as soon as istio configs + // are retrieved and update the view at a later time once the validations are fetched. + this.configsPromise.promise + .then(istioItems => { + this.configsPromise = makeCancelablePromise(validationPromises); + this.configsPromise.promise + .then(namespaceValidations => { + this.updateValidation(istioItems as IstioConfigItem[], namespaceValidations as NamespaceValidations); + this.forceUpdate(); + }) + .catch(istioError => { + if (!istioError.isCanceled) { + this.handleAxiosError('Could not fetch Istio objects list.', istioError); + } + }); + }) + .catch(() => undefined); + } } renderIstioItem(istioItem: IstioConfigItem, index: number) { @@ -319,6 +375,17 @@ class IstioConfigListComponent extends ListComponent.Component< ); return
{ruleListComponent}
; } + + private cancelAsyncs = () => { + if (this.nsPromise) { + this.nsPromise.cancel(); + this.nsPromise = undefined; + } + if (this.configsPromise) { + this.configsPromise.cancel(); + this.configsPromise = undefined; + } + }; } export default IstioConfigListComponent; diff --git a/src/pages/ServiceList/ItemDescription.tsx b/src/pages/ServiceList/ItemDescription.tsx index b729d2e45c..799e624c01 100644 --- a/src/pages/ServiceList/ItemDescription.tsx +++ b/src/pages/ServiceList/ItemDescription.tsx @@ -3,6 +3,7 @@ import { ServiceListItem } from '../../types/ServiceList'; import { ServiceHealth } from '../../types/Health'; import { DisplayMode, HealthIndicator } from '../../components/Health/HealthIndicator'; import ServiceErrorRate from './ServiceErrorRate'; +import { CancelablePromise, makeCancelablePromise } from '../../utils/Common'; interface Props { item: ServiceListItem; @@ -12,6 +13,8 @@ interface State { } export default class ItemDescription extends React.PureComponent { + private healthPromise?: CancelablePromise; + constructor(props: Props) { super(props); this.state = { health: undefined }; @@ -27,8 +30,24 @@ export default class ItemDescription extends React.PureComponent { } } + componentWillUnmount() { + if (this.healthPromise) { + this.healthPromise.cancel(); + this.healthPromise = undefined; + } + } + onItemChanged(item: ServiceListItem) { - 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() { diff --git a/src/pages/ServiceList/ServiceListComponent.tsx b/src/pages/ServiceList/ServiceListComponent.tsx index 2246b3a918..e157969551 100644 --- a/src/pages/ServiceList/ServiceListComponent.tsx +++ b/src/pages/ServiceList/ServiceListComponent.tsx @@ -18,7 +18,7 @@ import { ActiveFilter } from '../../types/Filters'; import { ServiceList, ServiceListItem } from '../../types/ServiceList'; import { IstioLogo } from '../../config'; import { authentication } from '../../utils/Authentication'; -import { removeDuplicatesArray } from '../../utils/Common'; +import { CancelablePromise, makeCancelablePromise, removeDuplicatesArray } from '../../utils/Common'; import RateIntervalToolbarItem from './RateIntervalToolbarItem'; import ItemDescription from './ItemDescription'; import { ListPage } from '../../components/ListPage/ListPage'; @@ -42,6 +42,10 @@ class ServiceListComponent extends ListComponent.Component< ServiceListComponentState, ServiceListItem > { + private nsPromise?: CancelablePromise>; + private servicesPromise?: CancelablePromise[]>; + private sortPromise?: CancelablePromise; + constructor(props: ServiceListComponentProps) { super(props); @@ -71,6 +75,10 @@ class ServiceListComponent extends ListComponent.Component< } } + componentWillUnmount() { + this.cancelAsyncs(); + } + paramsAreSynced(prevProps: ServiceListComponentProps) { return ( prevProps.pagination.page === this.props.pagination.page && @@ -87,10 +95,38 @@ class ServiceListComponent extends ListComponent.Component< }; sortItemList(services: ServiceListItem[], sortField: SortField, isAscending: boolean) { - return ServiceListFilters.sortServices(services, sortField, isAscending); + let lastSort: Promise; + const sorter = unsorted => { + this.sortPromise = makeCancelablePromise(ServiceListFilters.sortServices(services, 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 services to sort + // them and update the UI with the sorted list. + lastSort = sorter(services); + } else { + // If there is a "sortPromise", there may be an ongoing fetch/refresh. So, the received 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 fetchServices 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') @@ -100,7 +136,8 @@ class ServiceListComponent extends ListComponent.Component< 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.fetchServices( @@ -109,8 +146,13 @@ class ServiceListComponent extends ListComponent.Component< this.state.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.fetchServices(namespacesSelected, activeFilters, this.state.rateInterval, resetPagination); } @@ -131,30 +173,53 @@ class ServiceListComponent extends ListComponent.Component< fetchServices(namespaces: string[], filters: ActiveFilter[], rateInterval: number, resetPagination?: boolean) { const servicesPromises = namespaces.map(ns => API.getServices(authentication(), ns)); - Promise.all(servicesPromises).then(responses => { - const currentPage = resetPagination ? 1 : this.state.pagination.page; + this.servicesPromise = makeCancelablePromise(Promise.all(servicesPromises)); + this.servicesPromise.promise + .then(responses => { + const currentPage = resetPagination ? 1 : this.state.pagination.page; - let serviceListItems: ServiceListItem[] = []; - responses.forEach(response => { - ServiceListFilters.filterBy(response.data, filters); - serviceListItems = serviceListItems.concat(this.getServiceItem(response.data, rateInterval)); - }); - - ServiceListFilters.sortServices(serviceListItems, 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 serviceListItems: ServiceListItem[] = []; + responses.forEach(response => { + ServiceListFilters.filterBy(response.data, filters); + serviceListItems = serviceListItems.concat(this.getServiceItem(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( + ServiceListFilters.sortServices(serviceListItems, 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.servicesPromise = undefined; + }) + .catch(err => { + if (!err.isCanceled) { + console.debug(err); } - ); - }); + }); } render() { @@ -229,6 +294,21 @@ class ServiceListComponent extends ListComponent.Component< ); } + + private cancelAsyncs = () => { + if (this.nsPromise) { + this.nsPromise.cancel(); + this.nsPromise = undefined; + } + if (this.servicesPromise) { + this.servicesPromise.cancel(); + this.servicesPromise = undefined; + } + if (this.sortPromise) { + this.sortPromise.cancel(); + this.sortPromise = undefined; + } + }; } export default ServiceListComponent; diff --git a/src/pages/ServiceList/__tests__/ItemDescription.test.tsx b/src/pages/ServiceList/__tests__/ItemDescription.test.tsx index 3d58dbd3f5..d4a95c0421 100644 --- a/src/pages/ServiceList/__tests__/ItemDescription.test.tsx +++ b/src/pages/ServiceList/__tests__/ItemDescription.test.tsx @@ -24,15 +24,14 @@ describe('ItemDescription', () => { }; }); - it('should render with promise resolving', done => { + it('should render with promise resolving', () => { const wrapper = shallow(); expect(wrapper.text()).toBe(''); resolver(health); - item.healthPromise.then(() => { + return new Promise(r => setImmediate(r)).then(() => { wrapper.update(); expect(wrapper.text()).toBe('Health: '); - done(); }); }); }); diff --git a/src/pages/WorkloadList/ItemDescription.tsx b/src/pages/WorkloadList/ItemDescription.tsx index ae13c4be50..b6dd5a7f3c 100644 --- a/src/pages/WorkloadList/ItemDescription.tsx +++ b/src/pages/WorkloadList/ItemDescription.tsx @@ -7,6 +7,7 @@ import { Link } from 'react-router-dom'; import { WorkloadHealth } from '../../types/Health'; import { DisplayMode, HealthIndicator } from '../../components/Health/HealthIndicator'; import ErrorRate from './ErrorRate'; +import { CancelablePromise, makeCancelablePromise } from '../../utils/Common'; type ItemDescriptionState = { health?: WorkloadHealth; @@ -18,6 +19,8 @@ type ItemDescriptionProps = { }; class ItemDescription extends React.Component { + private healthPromise?: CancelablePromise; + constructor(props: ItemDescriptionProps) { super(props); this.state = { @@ -35,8 +38,24 @@ class ItemDescription extends React.Component 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() { diff --git a/src/pages/WorkloadList/WorkloadListComponent.tsx b/src/pages/WorkloadList/WorkloadListComponent.tsx index 65e61769fd..fc92243150 100644 --- a/src/pages/WorkloadList/WorkloadListComponent.tsx +++ b/src/pages/WorkloadList/WorkloadListComponent.tsx @@ -7,7 +7,7 @@ import { WorkloadListFilters } from './FiltersAndSorts'; 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 ItemDescription from './ItemDescription'; import RateIntervalToolbarItem from '../ServiceList/RateIntervalToolbarItem'; import { ListPage } from '../../components/ListPage/ListPage'; @@ -28,6 +28,10 @@ class WorkloadListComponent extends ListComponent.Component< WorkloadListComponentState, WorkloadListItem > { + private nsPromise?: CancelablePromise>; + private workloadsPromise?: CancelablePromise[]>; + private sortPromise?: CancelablePromise; + constructor(props: WorkloadListComponentProps) { super(props); this.state = { @@ -56,6 +60,10 @@ class WorkloadListComponent extends ListComponent.Component< } } + componentWillUnmount() { + this.cancelAsyncs(); + } + paramsAreSynced(prevProps: WorkloadListComponentProps) { return ( prevProps.pagination.page === this.props.pagination.page && @@ -72,10 +80,40 @@ class WorkloadListComponent extends ListComponent.Component< }; sortItemList(workloads: WorkloadListItem[], sortField: SortField, isAscending: boolean) { - return WorkloadListFilters.sortWorkloadsItems(workloads, sortField, isAscending); + let lastSort: Promise; + const sorter = unsorted => { + this.sortPromise = makeCancelablePromise( + WorkloadListFilters.sortWorkloadsItems(workloads, 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 workloads to sort + // them and update the UI with the sorted list. + lastSort = sorter(workloads); + } else { + // If there is a "sortPromise", there may be an ongoing fetch/refresh. So, the received 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 fetchWorkloads 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') @@ -85,12 +123,18 @@ class WorkloadListComponent extends ListComponent.Component< 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.fetchWorkloads(namespaces.map(namespace => namespace.name), activeFilters, 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.fetchWorkloads(namespacesSelected, activeFilters, resetPagination); } @@ -114,30 +158,56 @@ class WorkloadListComponent extends ListComponent.Component< fetchWorkloads(namespaces: string[], filters: ActiveFilter[], resetPagination?: boolean) { const workloadsConfigPromises = namespaces.map(namespace => API.getWorkloads(authentication(), namespace)); - Promise.all(workloadsConfigPromises).then(responses => { - const currentPage = resetPagination ? 1 : this.state.pagination.page; - let workloadsItems: WorkloadListItem[] = []; - responses.forEach(response => { - WorkloadListFilters.filterBy(response.data, filters); - workloadsItems = workloadsItems.concat(this.getDeploymentItems(response.data)); - }); - WorkloadListFilters.sortWorkloadsItems( - workloadsItems, - this.state.currentSortField, - this.state.isSortAscending - ).then(sorted => { - this.setState(prevState => { - return { - listItems: sorted, - pagination: { - page: currentPage, - perPage: prevState.pagination.perPage, - perPageOptions: ListPage.perPageOptions - } - }; + this.workloadsPromise = makeCancelablePromise(Promise.all(workloadsConfigPromises)); + this.workloadsPromise.promise + .then(responses => { + const currentPage = resetPagination ? 1 : this.state.pagination.page; + let workloadsItems: WorkloadListItem[] = []; + responses.forEach(response => { + WorkloadListFilters.filterBy(response.data, filters); + workloadsItems = workloadsItems.concat(this.getDeploymentItems(response.data)); }); + 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( + WorkloadListFilters.sortWorkloadsItems( + workloadsItems, + 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.workloadsPromise = undefined; + }) + .catch(err => { + if (!err.isCanceled) { + console.debug(err); + } }); - }); } render() { @@ -196,6 +266,21 @@ class WorkloadListComponent extends ListComponent.Component< ); } + + private cancelAsyncs = () => { + if (this.nsPromise) { + this.nsPromise.cancel(); + this.nsPromise = undefined; + } + if (this.workloadsPromise) { + this.workloadsPromise.cancel(); + this.workloadsPromise = undefined; + } + if (this.sortPromise) { + this.sortPromise.cancel(); + this.sortPromise = undefined; + } + }; } export default WorkloadListComponent;