Skip to content

Commit dd0fd50

Browse files
badvisionbadvision
authored
Merge pull request #56 from adobe/refactor-cleanup
Refactored code for clarity, adjusted timing in tests
2 parents 4161e43 + 3724a7b commit dd0fd50

File tree

3 files changed

+108
-49
lines changed

3 files changed

+108
-49
lines changed

‎index.js

Lines changed: 67 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -15,21 +15,41 @@ const AbortController = require('abort-controller');
1515
const fetch = require('node-fetch');
1616
const {FetchError} = fetch;
1717

18+
function getTimeRemaining(retryOptions) {
19+
if (retryOptions && retryOptions.startTime && retryOptions.retryMaxDuration) {
20+
const millisEllapsed = Date.now() - retryOptions.startTime;
21+
const remaining = retryOptions.retryMaxDuration - millisEllapsed;
22+
return Math.max(0, remaining);
23+
} else {
24+
return Infinity;
25+
}
26+
}
27+
28+
/**
29+
* Have we exceeded the max duration for this fetch operation?
30+
* @param {*} retryOptions Options including retryMaxDuration and startTime
31+
* @returns True if we have a max duration set and it is exceeded, otherwise false
32+
*/
33+
function isResponseTimedOut(retryOptions) {
34+
return getTimeRemaining(retryOptions) <= 0;
35+
}
36+
1837
/**
19-
* Retry
38+
* shouldRetry
2039
* @param {RetryOptions} retryOptions whether or not to retry on all http error codes or just >500
2140
* @param {Object} error error object if the fetch request returned an error
2241
* @param {Object} response fetch call response
42+
* @param {Number} wait Amount of time we will wait before retrying next
2343
* @returns {Boolean} whether or not to retry the request
2444
*/
25-
function retry(retryOptions, error, response) {
26-
if (retryOptions) {
27-
const totalMillisToWait = (retryOptions.retryInitialDelay) + (Date.now() - retryOptions.startTime);
28-
return ((totalMillisToWait < retryOptions.retryMaxDuration) &&
29-
(error !== null || (retryOptions.retryOnHttpResponse && (retryOptions.retryOnHttpResponse(response))))
30-
);
45+
function shouldRetry(retryOptions, error, response, waitTime) {
46+
if (getTimeRemaining(retryOptions) < waitTime) {
47+
return false;
48+
} else if (retryOptions && retryOptions.retryOnHttpResponse) {
49+
return retryOptions.retryOnHttpResponse(response);
50+
} else {
51+
return false;
3152
}
32-
return false;
3353
}
3454

3555
/**
@@ -84,7 +104,7 @@ function retryInit(options={}) {
84104
* @param {RetryOptions|Boolean} retryOptions Retry options
85105
* @param {Boolean} [random=true] Add randomness
86106
*/
87-
function retryDelay(retryOptions, random = true) {
107+
function getRetryDelay(retryOptions, random = true) {
88108
return retryOptions.retryInitialDelay +
89109
(random ? Math.floor(Math.random() * 100) : 99);
90110
}
@@ -147,43 +167,47 @@ module.exports = async function (url, options) {
147167

148168
return new Promise(function (resolve, reject) {
149169
const wrappedFetch = async () => {
150-
++attempt;
151-
152-
let timeoutHandler;
153-
if (retryOptions.socketTimeout) {
154-
const controller = new AbortController();
155-
timeoutHandler = setTimeout(() => controller.abort(), retryOptions.socketTimeout);
156-
options.signal = controller.signal;
157-
}
158-
159-
try {
160-
const response = await fetch(url, options);
161-
clearTimeout(timeoutHandler);
162-
163-
if (!retry(retryOptions, null, response)) {
164-
// response.timeout should reflect the actual timeout
165-
response.timeout = retryOptions.socketTimeout;
166-
return resolve(response);
167-
}
168-
169-
console.error(`Retrying in ${retryOptions.retryInitialDelay} milliseconds, attempt ${attempt - 1} failed (status ${response.status}): ${response.statusText}`);
170-
} catch (error) {
171-
clearTimeout(timeoutHandler);
172-
173-
if (!retry(retryOptions, error, null)) {
174-
if (error.name === 'AbortError') {
175-
return reject(new FetchError(`network timeout at ${url}`, 'request-timeout'));
170+
while (!isResponseTimedOut(retryOptions)) {
171+
++attempt;
172+
const waitTime = getRetryDelay(retryOptions);
173+
174+
let timeoutHandler;
175+
if (retryOptions.socketTimeout) {
176+
const controller = new AbortController();
177+
timeoutHandler = setTimeout(() => controller.abort(), retryOptions.socketTimeout);
178+
options.signal = controller.signal;
179+
}
180+
181+
try {
182+
const response = await fetch(url, options);
183+
184+
if (shouldRetry(retryOptions, null, response, waitTime)) {
185+
console.error(`Retrying in ${waitTime} milliseconds, attempt ${attempt} failed (status ${response.status}): ${response.statusText}`);
186+
} else {
187+
// response.timeout should reflect the actual timeout
188+
response.timeout = retryOptions.socketTimeout;
189+
return resolve(response);
176190
}
177-
178-
return reject(error);
191+
} catch (error) {
192+
const response = {status: error.code || 500, statusMessage : error.message || "Unknown server error"};
193+
if (!shouldRetry(retryOptions, error, response, waitTime)) {
194+
if (error.name === 'AbortError') {
195+
return reject(new FetchError(`network timeout at ${url}`, 'request-timeout'));
196+
} else {
197+
return reject(error);
198+
}
199+
}
200+
console.error(`Retrying in ${waitTime} milliseconds, attempt ${attempt} error: ${error.message}`);
201+
} finally {
202+
clearTimeout(timeoutHandler);
179203
}
180-
181-
console.error(`Retrying in ${retryOptions.retryInitialDelay} milliseconds, attempt ${attempt - 1} error: ${error.message}`);
204+
// Fetch loop is about to repeat, delay as needed first.
205+
if (waitTime > 0) {
206+
await new Promise(resolve => setTimeout(resolve, waitTime));
207+
}
208+
retryOptions.retryInitialDelay *= retryOptions.retryBackoff; // update retry interval
182209
}
183-
184-
retryOptions.retryInitialDelay *= retryOptions.retryBackoff; // update retry interval
185-
const waitTime = retryDelay(retryOptions);
186-
setTimeout(() => { wrappedFetch(); }, waitTime);
210+
reject(new FetchError(`network timeout at ${url}`, 'request-timeout'));
187211
};
188212
wrappedFetch();
189213
});

‎package-lock.json

Lines changed: 17 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎test/index.test.js

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,20 @@ const getPort = require('get-port');
2727
const FAKE_BASE_URL = 'https://fakeurl.com';
2828
const FAKE_PATH = '/image/test.png';
2929

30+
class Timer {
31+
constructor() {
32+
this.start = Date.now();
33+
}
34+
35+
get ellapsed() {
36+
return Date.now() - this.start;
37+
}
38+
39+
isBetween(v1, v2) {
40+
const answer = (this.ellapsed >= v1 && this.ellapsed <= v2);
41+
return answer;
42+
}
43+
}
3044

3145
describe('test `retryInit` function', () => {
3246
afterEach(() => {
@@ -355,7 +369,7 @@ describe('test fetch retry', () => {
355369
assert(nock.isDone());
356370
assert.strictEqual(response.statusText, 'OK');
357371
assert.strictEqual(response.status, 200);
358-
});
372+
}).timeout(3000);
359373

360374
it('test retry timeout on error 503', async () => {
361375
nock(FAKE_BASE_URL)
@@ -365,13 +379,17 @@ describe('test fetch retry', () => {
365379
message: 'something awful happened',
366380
code: '503',
367381
});
382+
const timer = new Timer();
368383
try {
369-
await fetch(`${FAKE_BASE_URL}${FAKE_PATH}`, { method: 'GET', retryOptions: { retryMaxDuration: 1000 } });
384+
await fetch(`${FAKE_BASE_URL}${FAKE_PATH}`, { method: 'GET', retryOptions: { retryMaxDuration: 700 } });
385+
assert.fail("Should have thrown an error!");
370386
} catch (e) {
371387
assert(nock.isDone());
372388
assert.strictEqual(e.message, 'request to https://fakeurl.com/image/test.png failed, reason: something awful happened');
373389
assert.strictEqual(e.code, '503');
374390
}
391+
console.log(`ellapsed: ${timer.ellapsed}`);
392+
assert.ok(timer.isBetween(300, 500), "Should have taken approximately 400ms");
375393
});
376394

377395
it('test retry timeout on 404 response', async () => {
@@ -383,7 +401,7 @@ describe('test fetch retry', () => {
383401
const response = await fetch(`${FAKE_BASE_URL}${FAKE_PATH}`, {
384402
method: 'GET',
385403
retryOptions: {
386-
retryMaxDuration: 1000,
404+
retryMaxDuration: 500,
387405
retryOnHttpResponse: (res) => { return !res.ok; }
388406
}
389407
});
@@ -397,13 +415,15 @@ describe('test fetch retry', () => {
397415
.get(FAKE_PATH)
398416
.twice()
399417
.reply(505);
418+
const timer = new Timer();
400419
const response = await fetch(`${FAKE_BASE_URL}${FAKE_PATH}`,
401420
{
402421
method: 'GET', retryOptions: {
403422
retryInitialDelay: 5000,
404423
retryMaxDuration: 1000
405424
}
406425
});
426+
assert.ok(timer.isBetween(0, 100), "Should have taken < 100ms");
407427
assert(!nock.isDone()); // should fail on first fetch call
408428
nock.cleanAll();
409429
assert.strictEqual(response.statusText, 'HTTP Version Not Supported');
@@ -717,6 +737,7 @@ describe('test fetch retry', () => {
717737

718738
const result = await fetch(`http://${hostname}:${port}`, { method: 'GET', retryOptions: retry });
719739
assert.ok(result.status === 200);
740+
console.error(`result timeout ${result.timeout} vs socketTimeout ${socketTimeout}`);
720741
assert.ok(result.timeout === socketTimeout);
721742

722743
server.close();

0 commit comments

Comments
 (0)