Skip to content
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.

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

feat: more information for exceed timeout error #839

Merged
merged 11 commits into from
Jun 2, 2020
11 changes: 9 additions & 2 deletions src/normalCalls/retries.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,9 @@ export function retryable(
let timeoutId: ReturnType<typeof setTimeout> | null;
let now = new Date();
let deadline: number;
const startTime = now.getTime();
if (retry.backoffSettings.totalTimeoutMillis) {
deadline = now.getTime() + retry.backoffSettings.totalTimeoutMillis;
deadline = startTime + retry.backoffSettings.totalTimeoutMillis;
}
let retries = 0;
const maxRetries = retry.backoffSettings.maxRetries!;
Expand All @@ -80,8 +81,14 @@ export function retryable(
function repeat() {
timeoutId = null;
if (deadline && now.getTime() >= deadline) {
const timeCost = now.getTime() - startTime;
const funcName = func.name;
const error = new GoogleError(
'Retry total timeout exceeded before any response was received'
`Function ${funcName} takes ${timeCost} milliseconds which exceeds retry total timeout ${
retry.backoffSettings.totalTimeoutMillis
} milliseconds before any response was received. The retry settings is ${JSON.stringify(
retry.backoffSettings
)}.`
);
error.code = Status.DEADLINE_EXCEEDED;
callback(error);
Expand Down
14 changes: 14 additions & 0 deletions test/unit/apiCallable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,20 @@ describe('retryable', () => {
});
});

it('retry fails for exceeding total timeout', done => {
const spy = sinon.spy(fail);
const apiCall = createApiCall(spy, settings);
apiCall({}, undefined, err => {
assert.ok(err instanceof GoogleError);
assert.strictEqual(
err.message,
'Function takes 100 milliseconds which exceeds retry total timeout 100 milliseconds before any response was received. The retry settings is {"initialRetryDelayMillis":0,"retryDelayMultiplier":0,"maxRetryDelayMillis":0,"initialRpcTimeoutMillis":0,"rpcTimeoutMultiplier":0,"maxRpcTimeoutMillis":0,"totalTimeoutMillis":100}.'
Copy link
Contributor

Choose a reason for hiding this comment

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

The function name is missing here - is it a test setup problem, or a problem with func.name in the code? Will it actually work with the real function call?

Copy link
Contributor

Choose a reason for hiding this comment

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

In general, this is a user facing error message, and I'm not sure we need to dump the whole JSON object here - maybe just the total timeout (the one that is compared to)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, the name of function name is empty here, actually not sure the function name is helpful because users might just want to know which API they are calling.
re: timeout setting: I will just print out the total timeout value.

);
assert.strictEqual(err!.code, status.DEADLINE_EXCEEDED);
done();
});
});

// maxRetries is unsupported, and intended for internal use only.
it('errors when totalTimeoutMillis and maxRetries set', done => {
const maxRetries = 5;
Expand Down