Skip to content

Commit 64b89a8

Browse files
thiyaguk09thiyaguk09
committed
refactor(storage): make onResponse async and improve error formatting for retry failures
1 parent 6b9fd1a commit 64b89a8

2 files changed

Lines changed: 189 additions & 25 deletions

File tree

‎handwritten/storage/src/resumable-upload.ts‎

Lines changed: 98 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1345,7 +1345,7 @@ export class Upload extends Writable {
13451345
},
13461346
};
13471347
const res = await this.authClient.request(combinedReqOpts);
1348-
const successfulRequest = this.onResponse(res);
1348+
const successfulRequest = await this.onResponse(res);
13491349
this.removeListener('error', errorCallback);
13501350

13511351
return successfulRequest ? res : null;
@@ -1354,7 +1354,7 @@ export class Upload extends Writable {
13541354
/**
13551355
* @return {bool} is the request good?
13561356
*/
1357-
private onResponse(resp: GaxiosResponse) {
1357+
private async onResponse(resp: GaxiosResponse) {
13581358
if (
13591359
resp.status !== 200 &&
13601360
this.retryOptions.retryableErrorFn!({
@@ -1363,7 +1363,7 @@ export class Upload extends Writable {
13631363
name: resp.statusText,
13641364
})
13651365
) {
1366-
this.attemptDelayedRetry(resp);
1366+
await this.attemptDelayedRetry(resp);
13671367
return false;
13681368
}
13691369

@@ -1386,9 +1386,7 @@ export class Upload extends Writable {
13861386

13871387
if (retryDelay <= 0) {
13881388
this.destroy(
1389-
new Error(
1390-
`Retry total time limit exceeded - ${JSON.stringify(resp.data)}`,
1391-
),
1389+
formatRetryError('Retry total time limit exceeded', resp),
13921390
);
13931391
return;
13941392
}
@@ -1409,9 +1407,7 @@ export class Upload extends Writable {
14091407
}
14101408
this.numRetries++;
14111409
} else {
1412-
this.destroy(
1413-
new Error(`Retry limit exceeded - ${JSON.stringify(resp.data)}`),
1414-
);
1410+
this.destroy(formatRetryError('Retry limit exceeded', resp));
14151411
}
14161412
}
14171413

@@ -1456,6 +1452,99 @@ export class Upload extends Writable {
14561452
}
14571453
}
14581454

1455+
function formatRetryError(
1456+
prefix: string,
1457+
resp: Pick<GaxiosResponse, 'data' | 'status'>,
1458+
): Error {
1459+
const parts: string[] = [];
1460+
1461+
if (resp.status !== undefined && !isNaN(resp.status)) {
1462+
parts.push(`status: ${resp.status}`);
1463+
}
1464+
1465+
const err = resp.data;
1466+
if (err !== undefined && err !== null) {
1467+
if (err instanceof Error) {
1468+
const gaxiosErr = err as GaxiosError;
1469+
const errParts: string[] = [];
1470+
if (gaxiosErr.message) {
1471+
errParts.push(gaxiosErr.message);
1472+
}
1473+
const status = gaxiosErr.status ?? gaxiosErr.response?.status;
1474+
if (status !== undefined && !isNaN(status) && status !== resp.status) {
1475+
errParts.push(`status: ${status}`);
1476+
}
1477+
const statusText = gaxiosErr.response?.statusText;
1478+
if (statusText) {
1479+
errParts.push(`statusText: ${statusText}`);
1480+
}
1481+
const responseData = gaxiosErr.response?.data;
1482+
if (responseData !== undefined && responseData !== null && responseData !== '') {
1483+
errParts.push(
1484+
`response: ${
1485+
typeof responseData === 'object'
1486+
? JSON.stringify(responseData)
1487+
: responseData
1488+
}`,
1489+
);
1490+
}
1491+
if (gaxiosErr.code) {
1492+
errParts.push(`code: ${gaxiosErr.code}`);
1493+
}
1494+
if (errParts.length > 0) {
1495+
parts.push(...errParts);
1496+
} else {
1497+
parts.push(gaxiosErr.toString() || gaxiosErr.name || 'Unknown Error');
1498+
}
1499+
} else if (typeof err === 'object') {
1500+
const errParts: string[] = [];
1501+
const gaxiosErrLike = err as any;
1502+
if (gaxiosErrLike.message) {
1503+
errParts.push(String(gaxiosErrLike.message));
1504+
}
1505+
const status = gaxiosErrLike.status ?? gaxiosErrLike.response?.status;
1506+
if (status !== undefined && !isNaN(status) && status !== resp.status) {
1507+
errParts.push(`status: ${status}`);
1508+
}
1509+
const statusText = gaxiosErrLike.response?.statusText;
1510+
if (statusText) {
1511+
errParts.push(`statusText: ${statusText}`);
1512+
}
1513+
const responseData = gaxiosErrLike.response?.data;
1514+
if (responseData !== undefined && responseData !== null && responseData !== '') {
1515+
errParts.push(
1516+
`response: ${
1517+
typeof responseData === 'object'
1518+
? JSON.stringify(responseData)
1519+
: responseData
1520+
}`,
1521+
);
1522+
}
1523+
if (gaxiosErrLike.code) {
1524+
errParts.push(`code: ${String(gaxiosErrLike.code)}`);
1525+
}
1526+
1527+
if (errParts.length > 0) {
1528+
parts.push(...errParts);
1529+
} else {
1530+
const stringified = JSON.stringify(err);
1531+
if (stringified && stringified !== '{}') {
1532+
parts.push(stringified);
1533+
}
1534+
}
1535+
} else if (typeof err === 'string') {
1536+
if (err !== '') {
1537+
parts.push(err);
1538+
}
1539+
} else {
1540+
parts.push(String(err));
1541+
}
1542+
}
1543+
1544+
const suffix = parts.join(' - ');
1545+
return new Error(`${prefix} - ${suffix || 'Unknown Error'}`);
1546+
}
1547+
14591548
export function upload(cfg: UploadConfig) {
14601549
return new Upload(cfg);
14611550
}

‎handwritten/storage/test/resumable-upload.ts‎

Lines changed: 91 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -2275,10 +2275,10 @@ describe('resumable-upload', () => {
22752275
describe('500s', () => {
22762276
const RESP = {status: 500, data: 'error message from server'};
22772277

2278-
it('should increase the retry count if less than limit', () => {
2278+
it('should increase the retry count if less than limit', async () => {
22792279
up.getRetryDelay = () => 1;
22802280
assert.strictEqual(up.numRetries, 0);
2281-
assert.strictEqual(up.onResponse(RESP), false);
2281+
assert.strictEqual(await up.onResponse(RESP), false);
22822282
assert.strictEqual(up.numRetries, 1);
22832283
});
22842284

@@ -2287,15 +2287,17 @@ describe('resumable-upload', () => {
22872287
up.destroy = (err: Error) => {
22882288
assert.strictEqual(
22892289
err.message,
2290-
`Retry limit exceeded - ${JSON.stringify(RESP.data)}`
2290+
`Retry limit exceeded - status: 500 - error message from server`,
22912291
);
22922292
done();
22932293
};
22942294

2295-
up.onResponse(RESP);
2296-
up.onResponse(RESP);
2297-
up.onResponse(RESP);
2298-
up.onResponse(RESP);
2295+
(async () => {
2296+
await up.onResponse(RESP);
2297+
await up.onResponse(RESP);
2298+
await up.onResponse(RESP);
2299+
await up.onResponse(RESP);
2300+
})().catch(done);
22992301
});
23002302

23012303
describe('exponential back off', () => {
@@ -2321,19 +2323,19 @@ describe('resumable-upload', () => {
23212323
assert(delay <= maxTime);
23222324

23232325
// make it keep retrying until the limit is reached
2324-
up.onResponse(RESP);
2326+
void up.onResponse(RESP);
23252327
};
23262328

23272329
up.on('error', (err: Error) => {
23282330
assert.strictEqual(up.numRetries, 3);
23292331
assert.strictEqual(
23302332
err.message,
2331-
`Retry limit exceeded - ${JSON.stringify(RESP.data)}`
2333+
`Retry limit exceeded - status: 500 - error message from server`,
23322334
);
23332335
done();
23342336
});
23352337

2336-
up.onResponse(RESP);
2338+
void up.onResponse(RESP);
23372339
clock.runAll();
23382340
});
23392341
});
@@ -2348,23 +2350,22 @@ describe('resumable-upload', () => {
23482350
assert.strictEqual(resp, RESP);
23492351
done();
23502352
});
2351-
up.onResponse(RESP);
2353+
void up.onResponse(RESP);
23522354
});
23532355

2354-
it('should return true', () => {
2356+
it('should return true', async () => {
23552357
up.getRetryDelay = () => 1;
2356-
assert.strictEqual(up.onResponse(RESP), true);
2358+
assert.strictEqual(await up.onResponse(RESP), true);
23572359
});
23582360

2359-
it('should handle a custom status code when passed a retry function', () => {
2361+
it('should handle a custom status code when passed a retry function', async () => {
23602362
up.getRetryDelay = () => 1;
23612363
const RESP = {status: 1000};
23622364
const customHandlerFunction = (err: ApiError) => {
23632365
return err.code === 1000;
23642366
};
23652367
up.retryOptions.retryableErrorFn = customHandlerFunction;
2366-
2367-
assert.strictEqual(up.onResponse(RESP), false);
2368+
assert.strictEqual(await up.onResponse(RESP), false);
23682369
});
23692370
});
23702371
});
@@ -2490,6 +2491,80 @@ describe('resumable-upload', () => {
24902491

24912492
up.attemptDelayedRetry({});
24922493
});
2494+
2495+
it('should include correct details for standard native Errors', done => {
2496+
up.numRetries = 3;
2497+
up.retryLimit = 3;
2498+
const nativeError = new Error('native connection issue');
2499+
2500+
up.on('error', (err: Error) => {
2501+
assert.strictEqual(
2502+
err.message,
2503+
'Retry limit exceeded - native connection issue',
2504+
);
2505+
done();
2506+
});
2507+
2508+
up.attemptDelayedRetry({
2509+
status: NaN,
2510+
data: nativeError,
2511+
});
2512+
});
2513+
2514+
it('should include correct details for custom errors with empty messages', done => {
2515+
up.numRetries = 3;
2516+
up.retryLimit = 3;
2517+
const customError = new Error('');
2518+
(customError as any).code = 'ERR_SOMETHING_SPECIAL';
2519+
2520+
up.on('error', (err: Error) => {
2521+
assert.strictEqual(
2522+
err.message,
2523+
'Retry limit exceeded - code: ERR_SOMETHING_SPECIAL',
2524+
);
2525+
done();
2526+
});
2527+
2528+
up.attemptDelayedRetry({
2529+
status: NaN,
2530+
data: customError,
2531+
});
2532+
});
2533+
2534+
it('should include correct details for GaxiosErrors with empty/missing response bodies', done => {
2535+
up.numRetries = 3;
2536+
up.retryLimit = 3;
2537+
2538+
const gaxiosError = new GaxiosError(
2539+
'Request failed with status code 429',
2540+
{
2541+
method: 'POST',
2542+
url: 'https://example.com',
2543+
} as any,
2544+
{
2545+
status: 429,
2546+
statusText: 'Too Many Requests',
2547+
data: '',
2548+
config: {},
2549+
headers: {},
2550+
} as any
2551+
);
2552+
2553+
up.on('error', (err: Error) => {
2554+
// Let's check for the presence of key details rather than an exact string if we are unsure of exact GaxiosError fields,
2555+
// or assert the expected string. Let's assert the expected string:
2556+
assert(err.message.includes('Retry limit exceeded'));
2557+
assert(err.message.includes('Request failed with status code 429'));
2558+
assert(err.message.includes('status: 429') || err.message.includes('code: 429'));
2559+
assert(err.message.includes('statusText: Too Many Requests'));
2560+
done();
2561+
});
2562+
2563+
up.attemptDelayedRetry({
2564+
status: NaN,
2565+
data: gaxiosError,
2566+
});
2567+
});
24932568
});
24942569

24952570
describe('PROTOCOL_REGEX', () => {

0 commit comments

Comments
 (0)