Replace httpStatus argument with LoaderResponse in config.shouldRetry #…

…5658
video-dev · Aug 23, 2023 · 47e4bb2 · 47e4bb2
1 parent 2e0bb0e
commit 47e4bb2
Show file tree

Hide file tree

Showing 6 changed files with 76 additions and 22 deletions.
diff --git a/api-extractor/report/hls.js.api.md b/api-extractor/report/hls.js.api.md
@@ -2954,7 +2954,7 @@ export type RetryConfig = {
     retryDelayMs: number;
     maxRetryDelayMs: number;
     backoff?: 'exponential' | 'linear';
-    shouldRetry?: (retryConfig: RetryConfig | null | undefined, retryCount: number, isTimeout: boolean, httpStatus: number | undefined, retry: boolean) => boolean;
+    shouldRetry?: (retryConfig: RetryConfig | null | undefined, retryCount: number, isTimeout: boolean, loaderResponse: LoaderResponse | undefined, retry: boolean) => boolean;
 };
 
 // Warning: (ae-missing-release-tag) "SourceBufferName" is part of the package's API, but it is missing a release tag (@alpha, @beta, @public, or @internal)

diff --git a/src/config.ts b/src/config.ts
@@ -26,6 +26,7 @@ import type {
   FragmentLoaderContext,
   Loader,
   LoaderContext,
+  LoaderResponse,
   PlaylistLoaderContext,
 } from './types/loader';
 
@@ -193,8 +194,8 @@ export type RetryConfig = {
     retryConfig: RetryConfig | null | undefined,
     retryCount: number,
     isTimeout: boolean,
-    httpStatus: number | undefined,
-    retry: boolean
+    loaderResponse: LoaderResponse | undefined,
+    retry: boolean,
   ) => boolean;
 };
 

diff --git a/src/controller/error-controller.ts b/src/controller/error-controller.ts
@@ -260,12 +260,11 @@ export default class ErrorController implements NetworkComponentAPI {
     const hls = this.hls;
     const retryConfig = getRetryConfig(hls.config.playlistLoadPolicy, data);
     const retryCount = this.playlistError++;
-    const httpStatus = data.response?.code;
     const retry = shouldRetry(
       retryConfig,
       retryCount,
       isTimeoutError(data),
-      httpStatus,
+      data.response,
     );
     if (retry) {
       return {
@@ -303,12 +302,11 @@ export default class ErrorController implements NetworkComponentAPI {
       if (data.details !== ErrorDetails.FRAG_GAP) {
         level.fragmentError++;
       }
-      const httpStatus = data.response?.code;
       const retry = shouldRetry(
         retryConfig,
         fragmentErrors,
         isTimeoutError(data),
-        httpStatus,
+        data.response,
       );
       if (retry) {
         return {

diff --git a/src/utils/error-helper.ts b/src/utils/error-helper.ts
@@ -1,6 +1,7 @@
-import { LoadPolicy, LoaderConfig, RetryConfig } from '../config';
 import { ErrorDetails } from '../errors';
-import { ErrorData } from '../types/events';
+import type { LoadPolicy, LoaderConfig, RetryConfig } from '../config';
+import type { ErrorData } from '../types/events';
+import type { LoaderResponse } from '../types/loader';
 
 export function isTimeoutError(error: ErrorData): boolean {
   switch (error.details) {
@@ -50,11 +51,12 @@ export function shouldRetry(
   retryConfig: RetryConfig | null | undefined,
   retryCount: number,
   isTimeout: boolean,
-  httpStatus?: number | undefined,
+  loaderResponse?: LoaderResponse | undefined,
 ): retryConfig is RetryConfig & boolean {
   if (!retryConfig) {
     return false;
   }
+  const httpStatus = loaderResponse?.code;
   const retry =
     retryCount < retryConfig.maxNumRetry &&
     (retryForHttpStatus(httpStatus) || !!isTimeout);
@@ -63,8 +65,8 @@ export function shouldRetry(
         retryConfig,
         retryCount,
         isTimeout,
-        httpStatus,
-        retry
+        loaderResponse,
+        retry,
       )
     : retry;
 }

diff --git a/src/utils/xhr-loader.ts b/src/utils/xhr-loader.ts
@@ -241,7 +241,12 @@ class XhrLoader implements Loader<LoaderContext> {
           const retryConfig = config.loadPolicy.errorRetry;
           const retryCount = stats.retry;
           // if max nb of retries reached or if http status between 400 and 499 (such error cannot be recovered, retrying is useless), return error
-          if (shouldRetry(retryConfig, retryCount, false, status)) {
+          const response: LoaderResponse = {
+            url: context.url,
+            data: undefined,
+            code: status,
+          };
+          if (shouldRetry(retryConfig, retryCount, false, response)) {
             this.retry(retryConfig);
           } else {
             logger.error(`${status} while loading ${context.url}`);

diff --git a/tests/unit/utils/error-helper.js b/tests/unit/utils/error-helper.js
@@ -5,29 +5,77 @@ describe('ErrorHelper', function () {
     const retryConfig = {
       maxNumRetry: 3,
     };
-    expect(shouldRetry(retryConfig, 3, false, '502')).to.be.false;
-    expect(shouldRetry(null, 3, false, '502')).to.be.false;
-    expect(shouldRetry(retryConfig, 2, false, '502')).to.be.true;
-    expect(shouldRetry(retryConfig, 2, false, '404')).to.be.false;
+    expect(
+      shouldRetry(retryConfig, 3, false, {
+        url: '',
+        data: undefined,
+        code: 502,
+      }),
+    ).to.be.false;
+    expect(
+      shouldRetry(null, 3, false, {
+        url: '',
+        data: undefined,
+        code: 502,
+      }),
+    ).to.be.false;
+    expect(
+      shouldRetry(retryConfig, 2, false, {
+        url: '',
+        data: undefined,
+        code: 502,
+      }),
+    ).to.be.true;
+    expect(
+      shouldRetry(retryConfig, 2, false, {
+        url: '',
+        data: undefined,
+        code: 404,
+      }),
+    ).to.be.false;
 
     retryConfig.shouldRetry = (
       _retryConfig,
       _retryCount,
       _isTimeout,
-      httpStatus,
-      retry
+      loaderResponse,
+      retry,
     ) => {
-      if (!retry && httpStatus === '404') {
+      if (!retry && loaderResponse?.code === 404) {
         return true;
       }
 
       return false;
     };
-    expect(shouldRetry(retryConfig, 5, false, '404', false)).to.be.true;
+    expect(
+      shouldRetry(
+        retryConfig,
+        5,
+        false,
+        {
+          url: '',
+          data: undefined,
+          code: 404,
+        },
+        false,
+      ),
+    ).to.be.true;
 
     retryConfig.shouldRetry = (retryConfig, retryCount) => {
       return retryConfig.maxNumRetry <= retryCount;
     };
-    expect(shouldRetry(retryConfig, 2, false, '502', true)).to.be.false;
+    expect(
+      shouldRetry(
+        retryConfig,
+        2,
+        false,
+        {
+          url: '',
+          data: undefined,
+          code: 502,
+        },
+        true,
+      ),
+    ).to.be.false;
   });
 });