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

FullJitterBackoffStrategy has the wrong argument checks #4724

Closed
liammoyn opened this issue Nov 29, 2023 · 3 comments · Fixed by #4809
Closed

FullJitterBackoffStrategy has the wrong argument checks #4724

liammoyn opened this issue Nov 29, 2023 · 3 comments · Fixed by #4809
Assignees
Labels
bug This issue is a bug. p2 This is a standard priority issue

Comments

@liammoyn
Copy link

Describe the bug

FullJitterBackoffStrategy takes a base delay and a max delay, the constructor checks if these values are >= 0. This check should actually be > 0 though because passing a 0 for either of these values will result in calling random.nextInt(0) on line 65, throwing an illegal argument exception when calculating the retry delay.

Expected Behavior

The retry strategy throws an exception when creating the strategy.

Current Behavior

The constructor passes and then an IllegalArgumentException is thrown on retry.

Reproduction Steps

Pass 0 for base delay or max delay parameters, have the client retry.

Possible Solution

Change the checks in the constructor from isNotNegative to isPositive

Additional Information/Context

No response

AWS Java SDK version used

2.0

JDK version used

17

Operating System and version

macOS 17.1.1

@liammoyn liammoyn added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Nov 29, 2023
@debora-ito debora-ito added the p2 This is a standard priority issue label Dec 5, 2023
@debora-ito
Copy link
Member

I can repro the IllegalArgumentException, although it's a weird use case to set zero to both base and max delays.

We'll fix the checks.

@debora-ito debora-ito removed the needs-triage This issue or PR still needs to be triaged. label Dec 5, 2023
@debora-ito debora-ito self-assigned this Dec 5, 2023
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@debora-ito
Copy link
Member

@liammoyn the fix was released as part of version 2.23.1, let us know if see issues after the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. p2 This is a standard priority issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants