Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Retry upload requests under certain conditions #210

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Retry upload requests under certain conditions #210

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
69 changes: 61 additions & 8 deletions javascript-client/src/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
import axios, { AxiosInstance, AxiosRequestConfig } from 'axios';
import axios, {
AxiosError,
AxiosInstance,
AxiosRequestConfig,
AxiosResponse,
} from 'axios';

// Description of a part from initializeUpload()
interface PartInfo {
Expand Down Expand Up @@ -36,6 +41,7 @@ export enum S3FileFieldProgressState {
Sending,
Finalizing,
Done,
Retrying,
}

export interface S3FileFieldProgress {
Expand All @@ -49,7 +55,41 @@ export type S3FileFieldProgressCallback = (progress: S3FileFieldProgress) => voi
export interface S3FileFieldClientOptions {
readonly baseUrl: string;
readonly onProgress?: S3FileFieldProgressCallback;
readonly apiConfig?: AxiosRequestConfig
readonly apiConfig?: AxiosRequestConfig;
}

function sleep(ms: number): Promise<void> {
return new Promise((resolve) => {
window.setTimeout(() => { resolve(); }, ms);
});
}

function shouldRetry(error: Error): boolean {
// We only retry requests under certain failure modes. Namely, either
// network errors, or a subset of HTTP error codes.
const axiosErr = (error as AxiosError);
return axiosErr.isAxiosError && (
!axiosErr.response
|| [429, 500, 502, 503, 504].includes(axiosErr.response.status)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not all 5xx errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danlamanna suggested this list, and I added one extra code to his suggestion. I think I prefer a whitelist of specific codes, such that any "unexpected" situation will break out of our retry loop, but I'm happy to expand that list if there are other codes you want to add.

Copy link
Contributor

Choose a reason for hiding this comment

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

429 makes sense, but I think we should retry the entire range of 5xx errors unless we know it's inappropriate. @danlamanna what do you think?

);
}

async function retry<T>(
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about using one of these:

I prefer https://www.npmjs.com/package/retry-axios , since it uses proper Axios interceptors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I'd prefer not to have to rewrite this, but if you think using one of those libraries is going to be a better experience, go ahead and make the decision.

Copy link
Contributor

@brianhelba brianhelba Mar 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Using https://www.npmjs.com/package/retry-axios has the potential to remove most of the code (and maintenance burden) here, I think it would be worth trying. @zachmullen If you don't have time, I'd be happy to try adding it.

Copy link

Choose a reason for hiding this comment

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

FWIW, I'm using axios-retry in NLI, and it works fine. retry-axios does look a little more refined though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brianhelba that would be great if you want to take a crack at it. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not 100% sure, but from the README it looks like axios-retry doesn't have a callback when retry is happening, but retry-axios does.

fn: () => Promise<T>, onRetry: () => void, condition: (error: Error) => boolean = shouldRetry,
interval = 5000,
): Promise<T> {
while (true) { // eslint-disable-line no-constant-condition
try {
return await fn(); // eslint-disable-line no-await-in-loop
} catch (error) {
if (condition(error)) {
onRetry();
await sleep(interval); // eslint-disable-line no-await-in-loop
} else {
throw error;
}
}
}
}

export default class S3FileFieldClient {
Expand Down Expand Up @@ -108,17 +148,23 @@ export default class S3FileFieldClient {
let fileOffset = 0;
for (const part of parts) {
const chunk = file.slice(fileOffset, fileOffset + part.size);
// eslint-disable-next-line no-await-in-loop
const response = await axios.put(part.upload_url, chunk, {
// eslint-disable-next-line @typescript-eslint/no-loop-func
// eslint-disable-next-line @typescript-eslint/no-loop-func, no-await-in-loop
const response = await retry<AxiosResponse>(() => axios.put(part.upload_url, chunk, {
onUploadProgress: (e) => {
this.onProgress({
uploaded: fileOffset + e.loaded,
total: file.size,
state: S3FileFieldProgressState.Sending,
});
},
}), () => { // eslint-disable-line @typescript-eslint/no-loop-func
this.onProgress({
uploaded: fileOffset,
total: file.size,
state: S3FileFieldProgressState.Retrying,
});
});

uploadedParts.push({
part_number: part.part_number,
size: part.size,
Expand All @@ -140,15 +186,18 @@ export default class S3FileFieldClient {
protected async completeUpload(
multipartInfo: MultipartInfo, parts: UploadedPart[],
): Promise<void> {
const response = await this.api.post('upload-complete/', {
const response = await retry<AxiosResponse>(() => this.api.post('upload-complete/', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since POST is not necessarily idempotent, do we know what happens if a client re-calls the endpoint mistakenly (perhaps due to the network dropping only the response)?

If this endpoint is actually idempotent, maybe we should change it to a PUT request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think either the endpoint should be idempotent, or if not, it should return 400 if a duplicate happens, which would break us out of the loop.

upload_signature: multipartInfo.upload_signature,
upload_id: multipartInfo.upload_id,
parts,
}), () => {
this.onProgress({ state: S3FileFieldProgressState.Retrying });
});
const { complete_url: completeUrl, body } = response.data;

// TODO support HTTP 200 error: https://github.com/girder/django-s3-file-field/issues/209
// Send the CompleteMultipartUpload operation to S3
await axios.post(completeUrl, body, {
await retry<AxiosResponse>(() => axios.post(completeUrl, body, {
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume that if this is called repeatedly, AWS will either:

  • idempotently succeed again, which is fine
  • return an error in the body, which is unrecoverable (and the retries should stop)

I don't think we need to do anything more here, but we should be sure to handle this with #209.

headers: {
// By default, Axios sets "Content-Type: application/x-www-form-urlencoded" on POST
// requests. This causes AWS's API to interpret the request body as additional parameters
Expand All @@ -157,6 +206,8 @@ export default class S3FileFieldClient {
// CompleteMultipartUpload docs.
'Content-Type': null,
},
}), () => {
this.onProgress({ state: S3FileFieldProgressState.Retrying });
});
}

Expand All @@ -168,8 +219,10 @@ export default class S3FileFieldClient {
* @param multipartInfo Signed information returned from /upload-complete/.
*/
protected async finalize(multipartInfo: MultipartInfo): Promise<string> {
const response = await this.api.post('finalize/', {
const response = await retry<AxiosResponse>(() => this.api.post('finalize/', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, do we know what happens when this is called repeatedly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hopefully either 200 OK or 400 bad request. I don't know for sure.

upload_signature: multipartInfo.upload_signature,
}), () => {
this.onProgress({ state: S3FileFieldProgressState.Retrying });
});
return response.data.field_value;
}
Expand Down