Skip to content

[wyndara][fix] maximum request retry time #17

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

Open
phillip-ng wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[wyndara][fix] maximum request retry time #17

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a17594d
fix: maximum retry time for consecutive failed requests
phillip-ng Aug 25, 2023
0f8d23f
fix: correct test for failure request
phillip-ng Aug 25, 2023
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
2 changes: 2 additions & 0 deletions src/constants/lib.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,5 @@ export const LIB_VERSION = '1.1.1';
export const PERSISTENCE_NAME = 'gotit.analytics.platform';

export const DISABLE_ERROR_CODE = 40101;

export const MAXIMUM_RETRY_TIME = 5;
49 changes: 43 additions & 6 deletions src/utils/__tests__/request.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import RequestHelper from '../request';
import { MAXIMUM_RETRY_TIME } from '../../constants/lib';

describe('utils/request', () => {
let instance;
Expand Down Expand Up @@ -28,11 +29,47 @@ describe('utils/request', () => {
let res = await instance.get();
expect(res).toEqual({ retry: false, data: response });

fetch.mockRejectedValue(response);
try {
res = await instance.get();
} catch (e) {
expect(e).toEqual({ retry: false, data: response });
}
fetch.mockReject(
new Error({
status: 500,
}),
);
res = await instance.get();
expect(res).toEqual({ retry: true, data: undefined });
});

it('should handle maximum retry times for consecutive failed requests', async () => {
// Mock first request to be success to reset retry time counter
fetch.mockResponse(JSON.stringify(response));
const successResponse = await instance.get();
expect(successResponse).toEqual({ retry: false, data: response });

const requestArr = new Array(MAXIMUM_RETRY_TIME - 1).fill();

// Next four requests are failed, able to retry
const results = await Promise.all(
requestArr.map(async () => {
fetch.mockReject(
new Error({
status: 500,
}),
);
const res = await instance.get();
return res;
}),
);

results.forEach((result) => {
expect(result).toEqual({ retry: true, data: undefined });
});

// Fifth request is failed, should not retry
fetch.mockReject(
new Error({
status: 500,
}),
);
const res = await instance.get();
expect(res).toEqual({ retry: false, data: undefined });
});
});
13 changes: 12 additions & 1 deletion src/utils/request.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
import CaseConverter from './caseConverter';
import { isEmpty } from './object';
import { MAXIMUM_RETRY_TIME } from '../constants/lib';

let retryTime = 0;
Copy link

@robertgotitapp robertgotitapp Aug 25, 2023

Choose a reason for hiding this comment

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

The retryTime variable is actually shared among different requests. Thus, when the request to this end point is being called 5-10 times in a very short time, these might cause unexpected effects on each other. Please review this scenario

Copy link
Author

@phillip-ng phillip-ng Aug 25, 2023

Choose a reason for hiding this comment

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

Can you provide me an example? Just make sure to understand what you're mentioning

Copy link

@robertgotitapp robertgotitapp Aug 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

For example, what happen if there are 6 requests to giap_javascript at the same time. What would happen?


const defaultHeaders = {
Accept: 'application/json',
Expand Down Expand Up @@ -33,8 +36,16 @@ const request = async (
? JSON.stringify(CaseConverter.camelCaseToSnakeCase(body))
: undefined,
});
retryTime = 0;
} catch (e) {
// pass
retryTime++;
if (retryTime >= MAXIMUM_RETRY_TIME) {
retryTime = 0;
return {
retry: false,
data: undefined,
};
}
}

if (!res || !res.status || res.status > 499) {
Expand Down