client: Control request cancellation in EntriesPage

This will allow us to reliably cancel the requests when new one starts, avoid memory leaks and also make the code generally cleaner. https://dev.to/viclafouch/cancel-properly-http-requests-in-react-hooks-and-avoid-memory-leaks-pd7
fossar · May 30, 2021 · 28d27f3 · 28d27f3
1 parent 38951f6
commit 28d27f3
Show file tree

Hide file tree

Showing 5 changed files with 31 additions and 35 deletions.
diff --git a/assets/js/helpers/ajax.js b/assets/js/helpers/ajax.js
@@ -73,7 +73,7 @@ export const liftToPromiseField = (wrapper) => (f) => (...params) => {
  * @return {controller: AbortController, promise: Promise}
  */
 export const makeAbortableFetch = (fetch) => (url, opts = {}) => {
-    let controller = new AbortController();
+    let controller = opts.abortController || new AbortController();
     let promise = fetch(url, {
         signal: controller.signal,
         ...opts

diff --git a/assets/js/requests/items.js b/assets/js/requests/items.js
@@ -78,18 +78,14 @@ function enrichItemsResponse(data) {
 /**
  * Get all items matching given filter.
  */
-export function getItems(filter) {
-    const { controller, promise } = ajax.get('', {
+export function getItems(filter, abortController) {
+    return ajax.get('', {
         body: ajax.makeSearchParams({
             ...filter,
             fromDatetime: filter.fromDatetime ? filter.fromDatetime.toISOString() : filter.fromDatetime
-        })
-    });
-
-    return {
-        controller,
-        promise: promise.then(response => response.json()).then(enrichItemsResponse)
-    };
+        }),
+        abortController,
+    }).promise.then(response => response.json()).then(enrichItemsResponse);
 }
 
 /**

diff --git a/assets/js/selfoss-base.js b/assets/js/selfoss-base.js
@@ -22,11 +22,6 @@ var selfoss = {
      */
     app: null,
 
-    /**
-     * instance of the currently running XHR that is used to reload the items list
-     */
-    activeAjaxReq: null,
-
     /**
      * the html title configured
      */

diff --git a/assets/js/selfoss-db-online.js b/assets/js/selfoss-db-online.js
@@ -230,17 +230,11 @@ selfoss.dbOnline = {
      *
      * @return void
      */
-    reloadList: function(fetchParams) {
-        if (selfoss.activeAjaxReq !== null) {
-            selfoss.activeAjaxReq.controller.abort();
-        }
-
-        selfoss.activeAjaxReq = itemsRequests.getItems({
+    reloadList: function(fetchParams, abortController) {
+        return itemsRequests.getItems({
             ...fetchParams,
             itemsPerPage: selfoss.config.itemsPerPage
-        });
-
-        let promise = selfoss.activeAjaxReq.promise.then((data) => {
+        }, abortController).then((data) => {
             selfoss.db.setOnline();
 
             if (!selfoss.db.enableOffline.value) {
@@ -264,19 +258,14 @@ selfoss.dbOnline = {
                 hasMore: data.hasMore
             };
         }).catch((error) => {
-            if (error.name == 'AbortError') {
+            if (error.name === 'AbortError') {
                 return;
             }
 
             return selfoss.handleAjaxError(error).then(function() {
                 return selfoss.dbOffline.reloadList(fetchParams);
             });
-        }).finally(() => {
-            // clean up
-            selfoss.activeAjaxReq = null;
         });
-
-        return promise;
     }
 
 

diff --git a/assets/js/templates/EntriesPage.jsx b/assets/js/templates/EntriesPage.jsx
@@ -13,7 +13,11 @@ import classNames from 'classnames';
 import { LocalizationContext } from '../helpers/i18n';
 import { HttpError } from '../errors';
 
-function reloadList({ fetchParams, append = false, waitForSync = true, entryId = null, setLoadingState }) {
+function reloadList({ fetchParams, abortController, append = false, waitForSync = true, entryId = null, setLoadingState }) {
+    if (abortController.signal.aborted) {
+        return Promise.resolve();
+    }
+
     if (entryId && fetchParams.fromId === undefined) {
         fetchParams = {
             ...fetchParams,
@@ -28,6 +32,10 @@ function reloadList({ fetchParams, append = false, waitForSync = true, entryId =
     setLoadingState(LoadingState.LOADING);
 
     var reload = () => {
+        if (abortController.signal.aborted) {
+            return Promise.resolve();
+        }
+
         let reloader = selfoss.dbOffline.reloadList;
 
         // tag, source and search filtering not supported offline (yet?)
@@ -49,7 +57,11 @@ function reloadList({ fetchParams, append = false, waitForSync = true, entryId =
         }
 
         setLoadingState(LoadingState.LOADING);
-        return reloader(fetchParams).then(({ entries, hasMore }) => {
+        return reloader(fetchParams, abortController).then(({ entries, hasMore }) => {
+            if (abortController.signal.aborted) {
+                return;
+            }
+
             setLoadingState(LoadingState.SUCCESS);
             selfoss.entriesPage.setHasMore(hasMore);
 
@@ -83,6 +95,10 @@ function reloadList({ fetchParams, append = false, waitForSync = true, entryId =
             }
 
         }).catch((error) => {
+            if (abortController.signal.aborted) {
+                return;
+            }
+
             if (error instanceof HttpError && error.response.status === 403) {
                 selfoss.history.push('/#');
                 // TODO: Use location state once we switch to BrowserRouter
@@ -165,6 +181,7 @@ export function EntriesPage({ entries, hasMore, loadingState, setLoadingState, s
     // Perform the scheduled reload.
     React.useEffect(() => {
         const append = fromId !== undefined || fromDatetime !== undefined;
+        const abortController = new AbortController();
 
         reloadList({
             // Object with parameters for GET /items and similar API calls
@@ -179,6 +196,7 @@ export function EntriesPage({ entries, hasMore, loadingState, setLoadingState, s
                 fromDatetime,
                 fromId,
             },
+            abortController,
             append,
             // We do not want to focus the entry on successive loads.
             entryId: append ? undefined : initialItemId,
@@ -194,9 +212,7 @@ export function EntriesPage({ entries, hasMore, loadingState, setLoadingState, s
         });
 
         return () => {
-            if (selfoss.activeAjaxReq !== null) {
-                selfoss.activeAjaxReq.controller.abort();
-            }
+            abortController.abort();
         };
     }, [params.filter, currentTag, currentSource, initialNavSourcesExpanded, searchText, fromDatetime, fromId, initialItemId, setLoadingState]);