Skip to content

Commit 616306e

Browse files
sindresorhussindresorhus
committed
Change retry callback order
Callback order changed from `onFailedAttempt` → `shouldConsumeRetry` → `shouldRetry` to `shouldConsumeRetry` → `onFailedAttempt` → `shouldRetry`, so consumption decisions are made before failure notifications and retry decisions. This aligns the callbacks with how retries are actually evaluated.
1 parent 96cce98 commit 616306e

4 files changed

Lines changed: 81 additions & 15 deletions

File tree

‎index.d.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ export type RetryContext = {
2222
This is calculated based on `minTimeout`, `factor`, `maxTimeout`, and `randomize` options.
2323
2424
Note: The actual delay may be shorter if it would exceed `maxRetryTime`.
25+
This is `0` when the retry is skipped or when no retry will occur.
2526
*/
2627
readonly retryDelay: number;
2728
};
@@ -30,7 +31,7 @@ export type Options = {
3031
/**
3132
Callback invoked on each failure. Receives a context object containing the error and retry state information.
3233
33-
The function is called before `shouldConsumeRetry` and `shouldRetry`, for all errors except `AbortError`.
34+
The function is called after `shouldConsumeRetry` and before `shouldRetry`, for all errors except `AbortError`.
3435
3536
The function is not called on `AbortError`.
3637
@@ -111,7 +112,7 @@ export type Options = {
111112
112113
When `false` is returned, the failure will not consume a retry or increment backoff values, but is still subject to `maxRetryTime`.
113114
114-
The function is called after `onFailedAttempt`, but before `shouldRetry`.
115+
The function is called before `onFailedAttempt` and `shouldRetry`.
115116
116117
The function is not called on `AbortError`.
117118

‎index.js‎

Lines changed: 27 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -81,16 +81,39 @@ async function onAttemptFailure({error, attemptNumber, retriesConsumed, startTim
8181
: options.retries;
8282

8383
const maxRetryTime = options.maxRetryTime ?? Number.POSITIVE_INFINITY;
84-
85-
// Calculate the delay upfront so it can be included in the context
8684
const delayTime = calculateDelay(retriesConsumed, options);
85+
const remainingTimeBeforeCallbacks = calculateRemainingTime(startTime, maxRetryTime);
86+
87+
if (remainingTimeBeforeCallbacks <= 0) {
88+
const context = Object.freeze({
89+
error: normalizedError,
90+
attemptNumber,
91+
retriesLeft,
92+
retriesConsumed,
93+
retryDelay: 0,
94+
});
8795

96+
await options.onFailedAttempt(context);
97+
98+
throw normalizedError;
99+
}
100+
101+
const consumeRetryContext = Object.freeze({
102+
error: normalizedError,
103+
attemptNumber,
104+
retriesLeft,
105+
retriesConsumed,
106+
retryDelay: retriesLeft > 0 ? delayTime : 0,
107+
});
108+
109+
const consumeRetry = await options.shouldConsumeRetry(consumeRetryContext);
110+
const effectiveDelay = consumeRetry && retriesLeft > 0 ? delayTime : 0;
88111
const context = Object.freeze({
89112
error: normalizedError,
90113
attemptNumber,
91114
retriesLeft,
92115
retriesConsumed,
93-
retryDelay: delayTime,
116+
retryDelay: effectiveDelay,
94117
});
95118

96119
await options.onFailedAttempt(context);
@@ -99,8 +122,6 @@ async function onAttemptFailure({error, attemptNumber, retriesConsumed, startTim
99122
throw normalizedError;
100123
}
101124

102-
const consumeRetry = await options.shouldConsumeRetry(context);
103-
104125
const remainingTime = calculateRemainingTime(startTime, maxRetryTime);
105126

106127
if (remainingTime <= 0 || retriesLeft <= 0) {
@@ -125,7 +146,7 @@ async function onAttemptFailure({error, attemptNumber, retriesConsumed, startTim
125146
return false;
126147
}
127148

128-
const finalDelay = Math.min(delayTime, remainingTime);
149+
const finalDelay = Math.min(effectiveDelay, remainingTime);
129150

130151
options.signal?.throwIfAborted();
131152

‎readme.md‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ Type: `Function`
5353

5454
Callback invoked on each failure. Receives a context object containing the error and retry state information.
5555

56-
The function is called _before_ `shouldConsumeRetry` and `shouldRetry`, for all errors _except_ `AbortError`.
56+
The function is called _after_ `shouldConsumeRetry` and _before_ `shouldRetry`, for all errors _except_ `AbortError`.
5757

5858
If the function throws, all retries will be aborted and the original promise will reject with the thrown error.
5959

@@ -88,7 +88,7 @@ The `context` object contains:
8888
- `attemptNumber` - The attempt number (starts at 1)
8989
- `retriesLeft` - Number of retries remaining
9090
- `retriesConsumed` - Number of retries consumed so far
91-
- `retryDelay` - The delay in milliseconds before the next retry (based on `minTimeout`, `factor`, `maxTimeout`, and `randomize`)
91+
- `retryDelay` - The delay in milliseconds before the next retry (based on `minTimeout`, `factor`, `maxTimeout`, and `randomize`). This is `0` when the retry is skipped or when no retry will occur.
9292

9393
The `onFailedAttempt` function can return a promise. For example, to add a [delay](https://github.com/sindresorhus/delay):
9494

@@ -138,7 +138,7 @@ Decide if this failure should consume a retry from the `retries` budget.
138138

139139
When `false` is returned, the failure will not consume a retry or increment backoff values, but is still subject to `maxRetryTime`.
140140

141-
The function is called _after_ `onFailedAttempt`, but before `shouldRetry`.
141+
The function is called _before_ `onFailedAttempt` and `shouldRetry`.
142142

143143
The function is _not_ called on `AbortError`.
144144

‎test.js‎

Lines changed: 48 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -268,12 +268,16 @@ test('shouldRetry controls retry behavior', async t => {
268268
t.is(index, 3);
269269
});
270270

271-
test('onFailedAttempt then shouldRetry order', async t => {
271+
test('shouldConsumeRetry then onFailedAttempt then shouldRetry order', async t => {
272272
const order = [];
273273

274274
await t.throwsAsync(pRetry(async () => {
275275
throw new Error('order');
276276
}, {
277+
shouldConsumeRetry() {
278+
order.push('shouldConsumeRetry');
279+
return true;
280+
},
277281
onFailedAttempt() {
278282
order.push('onFailedAttempt');
279283
},
@@ -283,7 +287,7 @@ test('onFailedAttempt then shouldRetry order', async t => {
283287
},
284288
}));
285289

286-
t.deepEqual(order, ['onFailedAttempt', 'shouldRetry']);
290+
t.deepEqual(order, ['shouldConsumeRetry', 'onFailedAttempt', 'shouldRetry']);
287291
});
288292

289293
test('handles async shouldRetry with maxRetryTime', async t => {
@@ -1266,6 +1270,24 @@ test('shouldConsumeRetry returning false still calls shouldRetry', async t => {
12661270
t.is(shouldRetryCalls, 1);
12671271
});
12681272

1273+
test('shouldConsumeRetry sees retryDelay as 0 when retriesLeft is 0', async t => {
1274+
let retryDelay;
1275+
1276+
await t.throwsAsync(pRetry(async () => {
1277+
throw new Error('fail');
1278+
}, {
1279+
retries: 0,
1280+
minTimeout: 100,
1281+
randomize: false,
1282+
shouldConsumeRetry(context) {
1283+
retryDelay = context.retryDelay;
1284+
return true;
1285+
},
1286+
}));
1287+
1288+
t.is(retryDelay, 0);
1289+
});
1290+
12691291
test.serial('retryDelay is provided in onFailedAttempt context', async t => {
12701292
const retryDelays = [];
12711293
const originalSetTimeout = setTimeout;
@@ -1291,8 +1313,30 @@ test.serial('retryDelay is provided in onFailedAttempt context', async t => {
12911313
},
12921314
));
12931315

1294-
// Verify retryDelay follows exponential backoff formula
1295-
t.deepEqual(retryDelays, [100, 200, 400, 800]);
1316+
// Verify retryDelay follows exponential backoff formula, ending with 0 when no retry remains
1317+
t.deepEqual(retryDelays, [100, 200, 400, 0]);
1318+
});
1319+
1320+
test.serial('retryDelay is zero when shouldConsumeRetry skips the retry', async t => {
1321+
const retryDelays = [];
1322+
1323+
await t.throwsAsync(pRetry(
1324+
async () => {
1325+
throw new Error('test');
1326+
},
1327+
{
1328+
retries: 2,
1329+
minTimeout: 100,
1330+
randomize: false,
1331+
shouldConsumeRetry: () => false,
1332+
shouldRetry: () => false,
1333+
onFailedAttempt({retryDelay}) {
1334+
retryDelays.push(retryDelay);
1335+
},
1336+
},
1337+
));
1338+
1339+
t.deepEqual(retryDelays, [0]);
12961340
});
12971341

12981342
test.serial('Only consumed retries advance backoff', async t => {

0 commit comments

Comments
 (0)