-
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
Avoid to have unexpected exception on status code 429 TooManyRequests #433
Conversation
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.
Some horizon instances do not populate the Retry-After header (horizon.stellar.org for example).
The SDK assumes this header will be present
+1 for improving the SDK to be resilient to this.
It's unclear to me what the impact of the default value used will be, and I've mentioned some folks for their input. @Ifropc I see you already approved, so if you're confident about the default value that's fine by me.
Aside of improving the SDK's resilience, the fact that horizon.stellar.org
isn't returning a header that is required by the SDK is very interesting and a potential bug in how horizon.stellar.org
is configured either in its application or infrastructure. cc @stellar/horizon-committers @mollykarcher @stellar/ops-team
@@ -28,7 +28,11 @@ public T handleResponse(final Response response) throws IOException, TooManyRequ | |||
try { | |||
// Too Many Requests | |||
if (response.code() == 429) { | |||
int retryAfter = Integer.parseInt(response.header("Retry-After")); | |||
|
|||
int retryAfter = -1; |
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
from TooManyRequestsException
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-After
The 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 on TooManyRequestsException.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.
0502185
to
b04dd35
Compare
b04dd35
to
09d7f45
Compare
@@ -28,7 +29,14 @@ public T handleResponse(final Response response) throws IOException, TooManyRequ | |||
try { | |||
// Too Many Requests | |||
if (response.code() == 429) { | |||
int retryAfter = Integer.parseInt(response.header("Retry-After")); | |||
|
|||
Optional<Integer> retryAfter = Optional.empty(); |
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.
per other comment related to not using Optional for inputs, retryAfter can just be Integer here.
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.
Can you elaborate on that? I personally prefer to work with Optional classes everywhere, instead of nullable types. To avoid NPE by mistake somewhere
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.
what's the purpose of the input parameter, either provide a value or none, can be accomplished with standard sdk, use an object(boxed primitives) and null as absent or do overloaded methods, using Optional as input parameter just adds extra layers that don't provide anything more, now you have to null check/guard on the optional first before referencing it to see if it's value is present/absent, it's duplicative. Optionals provide useful meta for return values, but not as input params. google java optional misuse
to see more on this line of thought.
@@ -28,7 +29,14 @@ public T handleResponse(final Response response) throws IOException, TooManyRequ | |||
try { | |||
// Too Many Requests | |||
if (response.code() == 429) { | |||
int retryAfter = Integer.parseInt(response.header("Retry-After")); | |||
|
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.
just for consideration, could removeheader
variable usage
Integer retryAfter = null;
try {
retryAfter = Integer.parseInt(response.header("Retry-After"));
} catch (Exception ignored) {}
@@ -1,5 +1,6 @@ | |||
package org.stellar.sdk.requests; | |||
|
|||
import com.google.common.base.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.
this looks like an un-used import now, does your IDE highlight/warn? if using Intellij, can do 'optimize imports', will re-order and remove unused.
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.
looks good, thanks for contributions!
Some horizon instances do not populate the Retry-After header (horizon.stellar.org for example).
The SDK assumes this header will be present and then throw NumberFormatException instead of the TooManyRequestsException.
This also affect dotnet and maybe other SDKs like flutter.