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.

Sign up for GitHub

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(util-waiter): add waiter utilities package #1736

Merged
merged 15 commits into from
Dec 8, 2020
Merged

feat(util-waiter): add waiter utilities package #1736

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
bbfc196
feat: set up waiters package
alexforsyth Dec 1, 2020
4f79dbb
feat: adding validation and sleep functions
alexforsyth Dec 1, 2020
2bf531f
feat: waiter interface
alexforsyth Dec 1, 2020
a620107
feat: generic forever polling
alexforsyth Dec 1, 2020
013953c
feat: exports
alexforsyth Dec 1, 2020
05acce5
fix: wording update for maxWaitTime
alexforsyth Dec 1, 2020
cd39e22
fix: clean up and updates per design review
alexforsyth Dec 3, 2020
7702314
polling not working as intended
alexforsyth Dec 3, 2020
6b74244
fix: poller.spec.ts
trivikr Dec 3, 2020
5f622b5
fix: adding tests to polling
alexforsyth Dec 3, 2020
40a6460
fix: empty changelog
alexforsyth Dec 3, 2020
1fc129f
fix: update naming to remove references to xml builder
alexforsyth Dec 3, 2020
2b52619
fix(util-watier): apply feedbacks
AllanZhengYP Dec 7, 2020
88ac270
feat(util-waiter): add jitter to waiters exponential backoff
AllanZhengYP Dec 7, 2020
4fbdb04
fix(util-waiter): rename exponentialBackoffWithJitter()
AllanZhengYP Dec 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/util-waiter/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
"description": "Shared utilities for client waiters for the AWS SDK",
"dependencies": {
"tslib": "^1.8.0",
"@aws-sdk/abort-controller": "1.0.0-rc.7"
"@aws-sdk/abort-controller": "1.0.0-rc.8"
},
"devDependencies": {
"@types/jest": "^26.0.4",
Expand Down
16 changes: 9 additions & 7 deletions packages/util-waiter/src/poller.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,12 @@ describe(runPolling.name, () => {

beforeEach(() => {
(sleep as jest.Mock).mockResolvedValueOnce("");
jest.spyOn(global.Math, "random").mockReturnValue(0.5);
});

afterEach(() => {
jest.clearAllMocks();
jest.spyOn(global.Math, "random").mockRestore();
});

it("should returns state in case of failure", async () => {
Expand Down Expand Up @@ -80,12 +82,12 @@ describe(runPolling.name, () => {

expect(sleep).toHaveBeenCalled();
expect(sleep).toHaveBeenCalledTimes(7);
expect(sleep).toHaveBeenNthCalledWith(1, 2);
expect(sleep).toHaveBeenNthCalledWith(2, 4);
expect(sleep).toHaveBeenNthCalledWith(3, 8);
expect(sleep).toHaveBeenNthCalledWith(4, 16);
expect(sleep).toHaveBeenNthCalledWith(5, 30);
expect(sleep).toHaveBeenNthCalledWith(6, 30);
expect(sleep).toHaveBeenNthCalledWith(7, 30);
expect(sleep).toHaveBeenNthCalledWith(1, 2); // min delay
expect(sleep).toHaveBeenNthCalledWith(2, 3); // +random() * 2
expect(sleep).toHaveBeenNthCalledWith(3, 5); // +random() * 4
expect(sleep).toHaveBeenNthCalledWith(4, 9); // +random() * 8
expect(sleep).toHaveBeenNthCalledWith(5, 17); // +random() * 16
expect(sleep).toHaveBeenNthCalledWith(6, 30); // max delay
expect(sleep).toHaveBeenNthCalledWith(7, 30); // max delay
});
});
18 changes: 10 additions & 8 deletions packages/util-waiter/src/poller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
import { sleep } from "./utils/sleep";
import { WaiterOptions, WaiterResult, WaiterState } from "./waiter";

function exponentialBackoff(floor: number, ciel: number, attempt: number): number {
return Math.min(ciel, floor * 2 ** attempt);
}
/**
* Reference: https://github.com/awslabs/smithy/pull/656
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to link to the waiters spec rather than the PR to update the spec. Maybe link here since this link will be relevant when we release the next version of Smithy that includes jitter in waiters: https://awslabs.github.io/smithy/1.0/spec/waiters.html#waiter-retries

* The theoretical limit to the attempt is max delay cannot be > Number.MAX_VALUE, but it's unlikely because of
* `maxWaitTime`
*/
const exponentialBackoff = (floor: number, ciel: number, attempt: number) =>
Math.floor(Math.min(ciel, randomInRange(floor, floor * 2 ** (attempt - 1))));
Copy link
Member

Choose a reason for hiding this comment

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

floor * 2 ** (attempt - 1)

I think this will overflow your number type for large values of attempt. To address this, you can compute the maximum attempt ceiling that doesn't exceed your ceiling.

Pseudo code:

attemptCeiling = (log(maxDelay / minDelay) / log(2)) + 1

if attempt > attemptCeiling:
    delay = maxDelay
else:
    delay = minDelay * 2 ** (attempt - 1)

delay = random(minDelay, delay)

if remainingTime - delay <= minDelay:
    delay = remainingTime - minDelay

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This

Copy link
Contributor

Choose a reason for hiding this comment

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

As I mentioned in the comment, this is not possible because it is in fact limited by maxWaitTime in the promise race in other abstraction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dont think Javascript has number overflow: https://www.algotech.solutions/blog/javascript/handle-number-overflow-javascript/

Copy link
Contributor

Choose a reason for hiding this comment

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

@alexforsyth It's possible to overflow the JS Number:

> Number.MAX_VALUE * 2
> Infinity

const randomInRange = (min: number, max: number) => min + Math.random() * (max - min);

/**
* Function that runs indefinite polling as part of waiters.
Expand All @@ -13,22 +18,19 @@ function exponentialBackoff(floor: number, ciel: number, attempt: number): numbe
* @param stateChecker function that checks the acceptor states on each poll.
*/
export const runPolling = async <T, S>(
params: WaiterOptions,
{ minDelay, maxDelay }: WaiterOptions,
client: T,
input: S,
acceptorChecks: (client: T, input: S) => Promise<WaiterResult>
): Promise<WaiterResult> => {
let currentAttempt = 1;
let currentDelay = params.minDelay;

while (true) {
await sleep(currentDelay);
await sleep(exponentialBackoff(minDelay, maxDelay, currentAttempt));
AllanZhengYP marked this conversation as resolved.
Show resolved Hide resolved
const { state } = await acceptorChecks(client, input);
if (state === WaiterState.SUCCESS || state === WaiterState.FAILURE) {
Copy link
Member

Choose a reason for hiding this comment

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

Probably better to say if (state !== WaiterState.RETRY) {

return { state };
}

currentDelay = exponentialBackoff(params.minDelay, params.maxDelay, currentAttempt);
currentAttempt += 1;
}
};