-
Notifications
You must be signed in to change notification settings - Fork 160
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
Avoid to have unexpected exception on status code 429 TooManyRequests #433
Merged
Ifropc
merged 7 commits into
lightsail-network:master
from
vinamogit:fix-npe-toomanyrequests
May 1, 2023
Merged
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
ffbf5f8
Avoid to have unexpected exception on status code 429 TooManyRequests
vinamogit ee44867
Make retryAfter to be optional
Ifropc 0ce3d55
Merge remote-tracking branch 'origin/master' into HEAD
Ifropc 09d7f45
Fix test
Ifropc 5487a36
Replace Java Optional with Guava
Ifropc 4033124
Use Integer
Ifropc 22d8d55
Optimize imports
Ifropc File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
65 changes: 65 additions & 0 deletions
65
src/test/java/org/stellar/sdk/requests/ResponseHandlerTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
package org.stellar.sdk.requests; | ||
|
||
import static org.junit.Assert.assertEquals; | ||
|
||
import java.io.IOException; | ||
|
||
import org.junit.Assert; | ||
import org.junit.Test; | ||
|
||
import okhttp3.OkHttpClient; | ||
import okhttp3.mockwebserver.MockResponse; | ||
import okhttp3.mockwebserver.MockWebServer; | ||
|
||
public class ResponseHandlerTest { | ||
|
||
@Test | ||
public void testTooManyRequests() throws IOException, InterruptedException { | ||
|
||
MockResponse response = new MockResponse(); | ||
response.setResponseCode(429); | ||
response.setHeader("Retry-After", "10"); | ||
|
||
MockWebServer mockWebServer = new MockWebServer(); | ||
mockWebServer.start(); | ||
mockWebServer.enqueue(response); | ||
|
||
OkHttpClient okHttpClient = new OkHttpClient().newBuilder().build(); | ||
try { | ||
|
||
AccountsRequestBuilder.execute(okHttpClient, mockWebServer.url("/")); | ||
Assert.fail(); | ||
} catch (TooManyRequestsException tmre) { | ||
assertEquals(10, tmre.getRetryAfter()); | ||
} finally { | ||
|
||
mockWebServer.shutdown(); | ||
mockWebServer.close(); | ||
} | ||
} | ||
|
||
@Test | ||
public void testTooManyRequestsNoHeader() throws IOException, InterruptedException { | ||
|
||
MockResponse response = new MockResponse(); | ||
response.setResponseCode(429); | ||
|
||
MockWebServer mockWebServer = new MockWebServer(); | ||
|
||
mockWebServer.start(); | ||
mockWebServer.enqueue(response); | ||
|
||
OkHttpClient okHttpClient = new OkHttpClient().newBuilder().build(); | ||
|
||
try { | ||
AccountsRequestBuilder.execute(okHttpClient, mockWebServer.url("/")); | ||
Assert.fail(); | ||
} catch (TooManyRequestsException tmre) { | ||
assertEquals(-1, tmre.getRetryAfter()); | ||
} finally { | ||
|
||
mockWebServer.shutdown(); | ||
mockWebServer.close(); | ||
} | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tamirms @stellar/anchor-eng Do you have an opinion on what the default value we should be able to use here and potential impact on applications that may be consuming it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My wondering is that because this value is always positive today, folks may be doing things like using the value as a duration in addition with current time to find a new time, or calling Time.sleep with the value, or any other number of things. Throwing a negative value into the mix may have unintended consequences and it may be safer to make the value an
Optional
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that returning an optional would be the best
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A reasonable default might be appropriate, such as 1000ms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the notion of a server or sdk provided retry interval introduces un-necessary coupling between the sdk and the client and a pattern to avoid, this could be the opportunity to remove
retryAfter
fromTooManyRequestsException
as a breaking api change to indicate clearer expectations to clients to determine their own error handling strategy(back-off algo, etc).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
Retry-After
header is a standard header to use to communicate how long a client needs to backoff for. Ref: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-AfterThe SDK is just providing access to that header, which seems reasonable. Developers can still determine their own error handling strategy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, making it optional is the best solution, as developer can decide default retry value themself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, changing to
Optional<Integer>
is good trade-off for handling retyAfter, it's worth introducing the breaking change that comes with it onTooManyRequestsException.getRetryAfter()
as one that will alert clients that were using it, to leverage the extra info provided in notPresent to decide how to handle.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree the ideal interface is an Optional, however a call needs to be made on whether breaking the API of the SDK is worth it. If not, which I suspect would be the case, using a reasonable default would be a great way to move forward with minimal impact.
Presumedly Horizon has a default/recommended value and we could use that same value here. They don't need to be the same, and it doesn't really matter if they change, as this is just a failsafe.
cc @mollykarcher I suspect you or your team can make a call if the Java SDK is accepting breaking API changes at the moment, or if a breaking change for this part of the API is outside any compatibility we'd like to guarantee.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm most concerned here that
horizon.stellar.org
isn't populating this header, which we'll look into and track separately in the #go monorepo. But as to whether we are accepting breaking changes to this SDK, technically this SDK is pre-1.0, so professes no backwards compatibility guarantees. I definitely think this is less than ideal, which is why I want to cut 1.0 soon (see #469) to cement expectations with developers. But given the current state of things, I think going with the breaking change here is acceptable.