Skip to content

Commit

Permalink
Fixing requestPromised
Browse files Browse the repository at this point in the history
  • Loading branch information
@mtrunkat
mtrunkat committed Nov 8, 2018
1 parent 4746a79 commit 18e2828
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 42 deletions.
2 changes: 1 addition & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "apify-client",
"version": "0.4.1",
"version": "0.4.2",
"description": "Apify API client for JavaScript",
"main": "build/index.js",
"keywords": [
Expand Down
44 changes: 22 additions & 22 deletions src/utils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,10 @@ export const requestPromise = async (options) => {
let error;

try {
const requestParams = Object.assign({}, options, { resolveWithFullResponse: true });
const requestParams = Object.assign({}, options, {
resolveWithFullResponse: true,
simple: false,
});
response = await request[method](requestParams); // eslint-disable-line
statusCode = response ? response.statusCode : null;
if (!statusCode || statusCode < 300) return options.resolveWithFullResponse ? response : response.body;
Expand All @@ -119,36 +122,33 @@ export const requestPromise = async (options) => {

// For status codes 300-499 except RATE_LIMIT_EXCEEDED_STATUS_CODE we immediately rejects the promise
// since it's probably caused by invalid url (redirect 3xx) or invalid user input (4xx).
if (statusCode >= 300 && statusCode < 500) {
if (statusCode >= 300 && statusCode < 500 && statusCode !== RATE_LIMIT_EXCEEDED_STATUS_CODE) {
throw newApifyClientErrorFromResponse(statusCode, response.body, isApiV1);
}

// If status code is >= 500 or RATE_LIMIT_EXCEEDED_STATUS_CODE then we repeat the request.
// We use exponential backoff alghorithm with up to `expBackOffMaxRepeats` repeats.
if (error || statusCode >= 500 || statusCode === RATE_LIMIT_EXCEEDED_STATUS_CODE) {
if (iteration >= expBackOffMaxRepeats) {
const errMessage = `Server request failed with ${iteration + 1} tries.`;
const errDetails = Object.assign(_.pick(options, 'url', 'method', 'qs'), {
hasBody: !!options.body,
iteration,
error,
statusCode,
});

throw new ApifyClientError(REQUEST_FAILED_ERROR_TYPE, errMessage, errDetails);
}
const errorDetails = Object.assign(_.pick(options, 'url', 'method', 'qs'), {
hasBody: !!options.body,
iteration,
error: error && error.message ? error.message : error,
statusCode,
});

// If one of these happened:
// - error occurd
// - status code is >= 500
// - RATE_LIMIT_EXCEEDED_STATUS_CODE
// then we repeat the request with exponential backoff alghorithm with up to `expBackOffMaxRepeats` repeats.
if (iteration >= expBackOffMaxRepeats) {
throw new ApifyClientError(REQUEST_FAILED_ERROR_TYPE, `Server request failed with ${iteration + 1} tries.`, errorDetails);
}

const waitMillis = expBackOffMillis * (2 ** iteration);
const randomizedWaitMillis = _.random(waitMillis, waitMillis * 2);
iteration++;

// Print warning when request is repeated 4 times.
if (iteration === Math.round(expBackOffMaxRepeats / 2)) {
log.warning(`Request failed ${iteration} times and will be repeated in ${waitMillis}ms`, {
hasBody: !!options.body,
statusCode,
iteration,
error,
});
log.warning(`Request failed ${iteration} times and will be repeated in ${waitMillis}ms`, errorDetails);
}

await promiseSleepMillis(randomizedWaitMillis); // eslint-disable-line
Expand Down
37 changes: 18 additions & 19 deletions test/utils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,13 +83,13 @@ describe('utils.newApifyClientErrorFromResponse()', () => {
describe('utils.requestPromise()', () => {
it('works as expected when request succeeds', () => {
const method = 'DELETE';
const opts = { method, foo: 'bar', promise: Promise };
const opts = { method, foo: 'bar' };
const expectedBody = { foo: 'something', bar: 123 };

const stub = sinon
.stub(request, method.toLowerCase())
.callsFake((passedOpts) => {
expect(passedOpts).to.be.eql(Object.assign({}, opts, { resolveWithFullResponse: true }));
expect(passedOpts).to.be.eql(Object.assign({}, opts, { resolveWithFullResponse: true, simple: false }));
return Promise.resolve({ body: expectedBody });
});

Expand All @@ -103,14 +103,14 @@ describe('utils.requestPromise()', () => {

it('works as expected with full response when request succeeds', () => {
const method = 'DELETE';
const opts = { method, foo: 'bar', resolveWithFullResponse: true, promise: Promise };
const opts = { method, foo: 'bar', resolveWithFullResponse: true };
const expectedBody = { foo: 'something', bar: 123 };
const expectedResponse = { statusCode: 123, foo: 'bar', body: expectedBody };

const stub = sinon
.stub(request, method.toLowerCase())
.callsFake((passedOpts) => {
expect(passedOpts).to.be.eql(Object.assign({}, opts, { resolveWithFullResponse: true }));
expect(passedOpts).to.be.eql(Object.assign({}, opts, { resolveWithFullResponse: true, simple: false }));
return Promise.resolve(expectedResponse);
});

Expand All @@ -134,7 +134,7 @@ describe('utils.requestPromise()', () => {
const stub = sinon
.stub(request, method.toLowerCase())
.callsFake((passedOpts) => {
expect(passedOpts).to.be.eql(Object.assign({}, opts, { resolveWithFullResponse: true }));
expect(passedOpts).to.be.eql(Object.assign({}, opts, { resolveWithFullResponse: true, simple: false }));
iteration++;
if (iteration < 8) return Promise.reject(new Error(errorMsg), null, {});
return Promise.resolve({ body: expectedBody });
Expand Down Expand Up @@ -165,7 +165,7 @@ describe('utils.requestPromise()', () => {
const stub = sinon
.stub(request, method.toLowerCase())
.callsFake((passedOpts) => {
expect(passedOpts).to.be.eql(Object.assign({}, opts, { resolveWithFullResponse: true }));
expect(passedOpts).to.be.eql(Object.assign({}, opts, { resolveWithFullResponse: true, simple: false }));
return Promise.reject(error);
});

Expand All @@ -178,10 +178,9 @@ describe('utils.requestPromise()', () => {
expect(err.type).to.be.eql(REQUEST_FAILED_ERROR_TYPE_V2);
expect(err.details.iteration).to.be.eql(opts.expBackOffMaxRepeats);
expect(err.details.statusCode).to.be.eql(undefined);
expect(err.details.error).to.be.eql(error);
expect(err.details.error).to.be.eql(error.message);
expect(err.details.hasBody).to.be.eql(false);
expect(err.details.method).to.be.eql(method);
expect(err.details.error).to.be.eql(error);
expect(err.details.url).to.be.eql('http://example.com/a/b');
expect(err.details.qs).to.be.eql({ foo: 'bar' });
stub.restore();
Expand All @@ -190,13 +189,13 @@ describe('utils.requestPromise()', () => {

it('works as expected when request throws an error for API V1', () => {
const method = 'POST';
const opts = { method, foo: 'bar', promise: Promise, isApiV1: true, expBackOffMaxRepeats: 8, expBackOffMillis: 5 };
const opts = { method, foo: 'bar', isApiV1: true, expBackOffMaxRepeats: 8, expBackOffMillis: 5 };
const error = new Error('some-error');

const stub = sinon
.stub(request, method.toLowerCase())
.callsFake((passedOpts) => {
expect(passedOpts).to.be.eql(Object.assign({}, opts, { resolveWithFullResponse: true }));
expect(passedOpts).to.be.eql(Object.assign({}, opts, { resolveWithFullResponse: true, simple: false }));
return Promise.reject(error);
});

Expand All @@ -209,22 +208,22 @@ describe('utils.requestPromise()', () => {
expect(err.type).to.be.eql(REQUEST_FAILED_ERROR_TYPE_V1);
expect(err.details.iteration).to.be.eql(opts.expBackOffMaxRepeats);
expect(err.details.statusCode).to.be.eql(undefined);
expect(err.details.error).to.be.eql(error);
expect(err.details.error).to.be.eql(error.message);
stub.restore();
});
});

it('works as expected when response contains error code and error details', () => {
const method = 'POST';
const opts = { method, foo: 'bar', promise: Promise };
const opts = { method, foo: 'bar' };
const type = 'SOME-TYPE';
const message = 'Some message';
const statusCode = 404;

const stub = sinon
.stub(request, method.toLowerCase())
.callsFake((passedOpts) => {
expect(passedOpts).to.be.eql(Object.assign({}, opts, { resolveWithFullResponse: true }));
expect(passedOpts).to.be.eql(Object.assign({}, opts, { resolveWithFullResponse: true, simple: false }));
return Promise.resolve({ statusCode, body: JSON.stringify({ type, message }) });
});

Expand All @@ -243,13 +242,13 @@ describe('utils.requestPromise()', () => {

it('works as expected when response contains only error code', () => {
const method = 'POST';
const opts = { method, foo: 'bar', promise: Promise };
const opts = { method, foo: 'bar' };
const statusCode = 404;

const stub = sinon
.stub(request, method.toLowerCase())
.callsFake((passedOpts) => {
expect(passedOpts).to.be.eql(Object.assign({}, opts, { resolveWithFullResponse: true }));
expect(passedOpts).to.be.eql(Object.assign({}, opts, { resolveWithFullResponse: true, simple: false }));
return Promise.resolve({ statusCode: 404, body: '' });
});

Expand Down Expand Up @@ -298,15 +297,15 @@ describe('utils.requestPromise()', () => {

it('supports exponential backoff', () => {
const method = 'DELETE';
const opts = { method, foo: 'bar', expBackOffMillis: 5, expBackOffMaxRepeats: 8, promise: Promise };
const opts = { method, foo: 'bar', expBackOffMillis: 5, expBackOffMaxRepeats: 8 };
const expectedBody = { foo: 'something', bar: 123 };

let iteration = 0;

const stub = sinon
.stub(request, method.toLowerCase())
.callsFake((passedOpts) => {
expect(passedOpts).to.be.eql(Object.assign({}, opts, { resolveWithFullResponse: true }));
expect(passedOpts).to.be.eql(Object.assign({}, opts, { resolveWithFullResponse: true, simple: false }));
iteration++;
if (iteration < 8) return Promise.resolve({ statusCode: 500 });
return Promise.resolve({ body: expectedBody });
Expand All @@ -323,15 +322,15 @@ describe('utils.requestPromise()', () => {

it('supports limit of exponential backoff iterations', () => {
const method = 'DELETE';
const opts = { method, foo: 'bar', expBackOffMillis: 5, expBackOffMaxRepeats: 3, promise: Promise };
const opts = { method, foo: 'bar', expBackOffMillis: 5, expBackOffMaxRepeats: 3 };
const expectedBody = { foo: 'something', bar: 123 };

let iteration = 0;

const stub = sinon
.stub(request, method.toLowerCase())
.callsFake((passedOpts) => {
expect(passedOpts).to.be.eql(Object.assign({}, opts, { resolveWithFullResponse: true }));
expect(passedOpts).to.be.eql(Object.assign({}, opts, { resolveWithFullResponse: true, simple: false }));
iteration++;
if (iteration <= 4) return Promise.resolve({ statusCode: 500 });
return Promise.resolve({ body: expectedBody });
Expand Down

0 comments on commit 18e2828

Please sign in to comment.