Skip to content

Commit

Permalink
Update retry rules to require http status or timeout error (depriorit…
Browse files Browse the repository at this point in the history
…izes retry on parse errors)
  • Loading branch information
robwalch committed Feb 25, 2023
1 parent 7cdbb32 commit 90984f9
Show file tree
Hide file tree
Showing 7 changed files with 32 additions and 14 deletions.
2 changes: 1 addition & 1 deletion src/controller/audio-stream-controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -515,7 +515,7 @@ class AudioStreamController
}
const details = track.details as LevelDetails;
if (!details) {
this.warn('Audio track details are defined on fragment load progress');
this.warn('Audio track details undefined on fragment load progress');
return;
}
const audioCodec =
Expand Down
2 changes: 1 addition & 1 deletion src/controller/base-stream-controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1397,7 +1397,7 @@ export default class BaseStreamController
} else if (retryConfig && errorAction) {
this.resetFragmentErrors(filterType);
if (retryCount < retryConfig.maxNumRetry) {
// Network retry is skipped for when level switch is preferred
// Network retry is skipped when level switch is preferred
errorAction.resolved = true;
} else {
logger.warn(
Expand Down
20 changes: 17 additions & 3 deletions src/controller/error-controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@ import {
NetworkErrorAction,
} from '../errors';
import { PlaylistContextType, PlaylistLevelType } from '../types/loader';
import { getRetryConfig, shouldRetry } from '../utils/error-helper';
import {
getRetryConfig,
isTimeoutError,
shouldRetry,
} from '../utils/error-helper';
import { HdcpLevels } from '../types/level';
import { logger } from '../utils/logger';
import type Hls from '../hls';
Expand Down Expand Up @@ -187,7 +191,12 @@ export default class ErrorController implements NetworkComponentAPI {
const retryConfig = getRetryConfig(hls.config.playlistLoadPolicy, data);
const retryCount = this.playlistError++;
const httpStatus = data.response?.code;
const retry = shouldRetry(retryConfig, retryCount, httpStatus);
const retry = shouldRetry(
retryConfig,
retryCount,
isTimeoutError(data),
httpStatus
);
if (retry) {
return {
action: NetworkErrorAction.RetryRequest,
Expand Down Expand Up @@ -232,7 +241,12 @@ export default class ErrorController implements NetworkComponentAPI {
if (level) {
level.fragmentError++;
const httpStatus = data.response?.code;
const retry = shouldRetry(retryConfig, fragmentErrors, httpStatus);
const retry = shouldRetry(
retryConfig,
fragmentErrors,
isTimeoutError(data),
httpStatus
);
if (retry) {
return {
action: NetworkErrorAction.RetryRequest,
Expand Down
1 change: 1 addition & 0 deletions src/types/loader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ export interface LoaderResponse {
url: string;
data?: string | ArrayBuffer | Object;
// Errors can include HTTP status code and error message
// Successful responses should include status code 200
code?: number;
text?: string;
}
Expand Down
9 changes: 4 additions & 5 deletions src/utils/error-helper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,18 +49,17 @@ export function getLoaderConfigWithoutReties(
export function shouldRetry(
retryConfig: RetryConfig | null | undefined,
retryCount: number,
isTimeout: boolean,
httpStatus?: number | undefined
): retryConfig is RetryConfig & boolean {
return (
!!retryConfig &&
retryCount < retryConfig.maxNumRetry &&
retryForHttpStatus(httpStatus)
(retryForHttpStatus(httpStatus) || !!isTimeout)
);
}

export function retryForHttpStatus(httpStatus: number | undefined) {
return (
httpStatus === undefined ||
(httpStatus !== 0 && (httpStatus < 400 || httpStatus > 499))
);
// Do not retry on status 4xx, status 0 (CORS error), or undefined (decrypt/gap/parse error)
return !!httpStatus && (httpStatus < 400 || httpStatus > 499);
}
4 changes: 3 additions & 1 deletion src/utils/fetch-loader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
LoaderStats,
LoaderConfiguration,
LoaderOnProgress,
LoaderResponse,
} from '../types/loader';
import { LoadStats } from '../loader/load-stats';
import ChunkCache from '../demux/chunk-cache';
Expand Down Expand Up @@ -151,9 +152,10 @@ class FetchLoader implements Loader<LoaderContext> {
stats.loaded = stats.total = total;
}

const loaderResponse = {
const loaderResponse: LoaderResponse = {
url: response.url,
data: responseData,
code: response.status,
};

if (onProgress && !Number.isFinite(config.highWaterMark)) {
Expand Down
8 changes: 5 additions & 3 deletions src/utils/xhr-loader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import type {
LoaderStats,
Loader,
LoaderConfiguration,
LoaderResponse,
} from '../types/loader';
import { LoadStats } from '../loader/load-stats';
import { RetryConfig } from '../config';
Expand Down Expand Up @@ -197,17 +198,18 @@ class XhrLoader implements Loader<LoaderContext> {
if (!this.callbacks) {
return;
}
const response = {
const response: LoaderResponse = {
url: xhr.responseURL,
data: data,
code: status,
};

this.callbacks.onSuccess(response, stats, context, xhr);
} else {
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, status)) {
if (shouldRetry(retryConfig, retryCount, false, status)) {
this.retry(retryConfig);
} else {
logger.error(`${status} while loading ${context.url}`);
Expand All @@ -226,7 +228,7 @@ class XhrLoader implements Loader<LoaderContext> {
loadtimeout(): void {
const retryConfig = this.config?.loadPolicy.timeoutRetry;
const retryCount = this.stats.retry;
if (shouldRetry(retryConfig, retryCount)) {
if (shouldRetry(retryConfig, retryCount, true)) {
this.retry(retryConfig);
} else {
logger.warn(`timeout while loading ${this.context.url}`);
Expand Down

0 comments on commit 90984f9

Please sign in to comment.