Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
151 changes: 144 additions & 7 deletions src/utils/api-request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -166,8 +166,66 @@ export class HttpError extends Error {
}
}

/**
* Specifies how failing HTTP requests should be retried.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please explain what the different fields mean in this interface (an example would be helpful). For example maxDelayInMillis is per retry and not in total per request, etc. backoff factor in seconds, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

*/
export interface RetryConfig {
maxRetries: number;
statusCodes?: number[];
ioErrorCodes?: string[];
backOffFactor?: number;
maxDelayInMillis: number;
}

/**
* Default retry configuration for HTTP requests. Retries once on connection reset and timeout errors.
*/
const DEFAULT_RETRY_CONFIG: RetryConfig = {
maxRetries: 1,
ioErrorCodes: ['ECONNRESET', 'ETIMEDOUT'],
maxDelayInMillis: 60 * 1000,
};

/**
* Ensures that the given RetryConfig object is valid.
*
* @param retry The configuration to be validated.
*/
function validateRetryConfig(retry: RetryConfig) {
if (!validator.isNumber(retry.maxRetries) || retry.maxRetries < 0) {
throw new FirebaseAppError(
AppErrorCodes.INVALID_ARGUMENT, 'maxRetries must be a non-negative integer');
}

if (typeof retry.backOffFactor !== 'undefined') {
if (!validator.isNumber(retry.backOffFactor) || retry.backOffFactor < 0) {
throw new FirebaseAppError(
AppErrorCodes.INVALID_ARGUMENT, 'backOffFactor must be a non-negative number');
}
}

if (!validator.isNumber(retry.maxDelayInMillis) || retry.maxDelayInMillis < 0) {
throw new FirebaseAppError(
AppErrorCodes.INVALID_ARGUMENT, 'maxDelayInMillis must be a non-negative integer');
}

if (typeof retry.statusCodes !== 'undefined' && !validator.isArray(retry.statusCodes)) {
throw new FirebaseAppError(AppErrorCodes.INVALID_ARGUMENT, 'statusCodes must be an array');
}

if (typeof retry.ioErrorCodes !== 'undefined' && !validator.isArray(retry.ioErrorCodes)) {
throw new FirebaseAppError(AppErrorCodes.INVALID_ARGUMENT, 'ioErrorCodes must be an array');
}
}

export class HttpClient {

constructor(private readonly retry: RetryConfig = DEFAULT_RETRY_CONFIG) {
if (this.retry) {
validateRetryConfig(this.retry);
}
}

/**
* Sends an HTTP request to a remote server. If the server responds with a successful response (2xx), the returned
* promise resolves with an HttpResponse. If the server responds with an error (3xx, 4xx, 5xx), the promise rejects
Expand All @@ -179,28 +237,38 @@ export class HttpClient {
* header should be explicitly set by the caller. To send a JSON leaf value (e.g. "foo", 5), parse it into JSON,
* and pass as a string or a Buffer along with the appropriate content-type header.
*
* @param {HttpRequest} request HTTP request to be sent.
* @param {HttpRequest} config HTTP request to be sent.
* @return {Promise<HttpResponse>} A promise that resolves with the response details.
*/
public send(config: HttpRequestConfig): Promise<HttpResponse> {
return this.sendWithRetry(config);
}

/**
* Sends an HTTP request, and retries it once in case of low-level network errors.
* Sends an HTTP request. In the event of an error, retries the HTTP request according to the
* RetryConfig set on the HttpClient.
*
* @param {HttpRequestConfig} config HTTP request to be sent.
* @param {number} retryAttempts Number of retries performed up to now.
* @return {Promise<HttpResponse>} A promise that resolves with the response details.
*/
private sendWithRetry(config: HttpRequestConfig, attempts: number = 0): Promise<HttpResponse> {
private sendWithRetry(config: HttpRequestConfig, retryAttempts: number = 0): Promise<HttpResponse> {
return AsyncHttpCall.invoke(config)
.then((resp) => {
return this.createHttpResponse(resp);
}).catch((err: LowLevelError) => {
const retryCodes = ['ECONNRESET', 'ETIMEDOUT'];
if (retryCodes.indexOf(err.code) !== -1 && attempts === 0) {
return this.sendWithRetry(config, attempts + 1);
})
.catch((err: LowLevelError) => {
const [delayMillis, canRetry] = this.getRetryDelayMillis(retryAttempts, err);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The private functions added do not provide enough descriptions on what is being computed or a summary of the underlying logic making it harder to deduce the underlying behavior. It helps to either add more comments here or descriptions of the private functions below.

Copy link
Contributor Author

@hiranya911 hiranya911 May 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think getRetryDelayMillis() is the confusing one, although I feel that the variable names explain what's going on. In any case, I've added a jsdoc comment to the method to further explain it.

if (canRetry && delayMillis <= this.retry.maxDelayInMillis) {
return this.waitForRetry(delayMillis).then(() => {
return this.sendWithRetry(config, retryAttempts + 1);
});
}

if (err.response) {
throw new HttpError(this.createHttpResponse(err.response));
}

if (err.code === 'ETIMEDOUT') {
throw new FirebaseAppError(
AppErrorCodes.NETWORK_TIMEOUT,
Expand All @@ -218,6 +286,75 @@ export class HttpClient {
}
return new DefaultHttpResponse(resp);
}

private waitForRetry(delayMillis: number): Promise<void> {
if (delayMillis > 0) {
return new Promise((resolve) => {
setTimeout(resolve, delayMillis);
});
}
return Promise.resolve();
}

private getRetryDelayMillis(retryAttempts: number, err: LowLevelError): [number, boolean] {
if (!this.isRetryEligible(retryAttempts, err)) {
return [0, false];
}
const response = err.response;
if (response && response.headers['retry-after']) {
const delayMillis = this.parseRetryAfterIntoMillis(response.headers['retry-after']);
if (delayMillis > 0) {
return [delayMillis, true];
}
}

return [this.backOffDelayMillis(retryAttempts), true];
}

private isRetryEligible(retryAttempts: number, err: LowLevelError): boolean {
if (!this.retry) {
return false;
}

if (retryAttempts >= this.retry.maxRetries) {
return false;
}

if (err.response) {
const statusCodes = this.retry.statusCodes || [];
return statusCodes.indexOf(err.response.status) !== -1;
}

const retryCodes = this.retry.ioErrorCodes || [];
return retryCodes.indexOf(err.code) !== -1;
}

/**
* Parses the Retry-After HTTP header as a milliseconds value. Return value is negative if the Retry-After header
* contains an expired timestamp or otherwise malformed.
*/
private parseRetryAfterIntoMillis(retryAfter: string): number {
const delaySeconds: number = parseInt(retryAfter, 10);
if (!isNaN(delaySeconds)) {
return delaySeconds * 1000;
}

const date = new Date(retryAfter);
if (!isNaN(date.getTime())) {
return date.getTime() - Date.now();
}
return -1;
}

private backOffDelayMillis(retryAttempts: number): number {
if (retryAttempts === 0) {
return 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you not wait on the first retry?
You will end up with a wait pattern: first fail, 0, 2000, 4000
Instead of: first fail, 1000, 2000, 4000
I think the latter has a better likelihood of succeeding with less time overall.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to retain the existing behavior, and also to align with how retries are implemented in other languages/libraries. Basically we only delay retries in the event of consecutive errors. Here's a similar implementation from Python's urllib3 package: https://github.com/urllib3/urllib3/blob/master/src/urllib3/util/retry.py#L222

In general, the existing strategy of immediately retrying on the first error has worked well for us so far. This is particularly useful in environments like GCF where low-level transient errors seem to be common.

}

const backOffFactor = this.retry.backOffFactor || 0;
const delayInSeconds = (2 ** retryAttempts) * backOffFactor;
return Math.min(delayInSeconds * 1000, this.retry.maxDelayInMillis);
}
}

/**
Expand Down
1 change: 1 addition & 0 deletions src/utils/error.ts
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,7 @@ export class FirebaseProjectManagementError extends PrefixedFirebaseError {
export class AppErrorCodes {
public static APP_DELETED = 'app-deleted';
public static DUPLICATE_APP = 'duplicate-app';
public static INVALID_ARGUMENT = 'invalid-argument';
public static INTERNAL_ERROR = 'internal-error';
public static INVALID_APP_NAME = 'invalid-app-name';
public static INVALID_APP_OPTIONS = 'invalid-app-options';
Expand Down
Loading