Avoid to have unexpected exception on status code 429 TooManyRequests #433

vinamogit · 2022-05-22T11:06:50Z

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.

leighmcculloch

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

leighmcculloch · 2023-04-28T18:35:36Z

src/main/java/org/stellar/sdk/requests/ResponseHandler.java

@@ -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;


@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?

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.

I agree that returning an optional would be the best

A reasonable default might be appropriate, such as 1000ms.

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).

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.

IMO, making it optional is the best solution, as developer can decide default retry value themself

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.

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.

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.

build.gradle

sreuland · 2023-04-28T20:57:11Z

src/main/java/org/stellar/sdk/requests/ResponseHandler.java

@@ -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();


per other comment related to not using Optional for inputs, retryAfter can just be Integer here.

Can you elaborate on that? I personally prefer to work with Optional classes everywhere, instead of nullable types. To avoid NPE by mistake somewhere

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.

sreuland · 2023-04-28T23:14:16Z

src/main/java/org/stellar/sdk/requests/ResponseHandler.java

@@ -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"));
+


just for consideration, could removeheader variable usage

Integer retryAfter = null; try { retryAfter = Integer.parseInt(response.header("Retry-After")); } catch (Exception ignored) {}

sreuland · 2023-04-28T23:16:41Z

src/main/java/org/stellar/sdk/requests/ResponseHandler.java

@@ -1,5 +1,6 @@
 package org.stellar.sdk.requests;

+import com.google.common.base.Optional;


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.

sreuland

looks good, thanks for contributions!

Avoid to have unexpected exception on status code 429 TooManyRequests

ffbf5f8

vinamogit mentioned this pull request May 22, 2022

Potential error for missing Retry-After header elucidsoft/dotnet-stellar-sdk#362

Closed

Ifropc requested review from leighmcculloch and Ifropc April 28, 2023 17:14

Ifropc approved these changes Apr 28, 2023

View reviewed changes

paulormnas mentioned this pull request Apr 28, 2023

CRASH when handling 429s #476

Closed

leighmcculloch reviewed Apr 28, 2023

View reviewed changes

Make retryAfter to be optional

ee44867

Ifropc force-pushed the fix-npe-toomanyrequests branch 2 times, most recently from 0502185 to b04dd35 Compare April 28, 2023 20:32

Ifropc added 2 commits April 28, 2023 13:33

Merge remote-tracking branch 'origin/master' into HEAD

0ce3d55

Fix test

09d7f45

Ifropc force-pushed the fix-npe-toomanyrequests branch from b04dd35 to 09d7f45 Compare April 28, 2023 20:33

sreuland reviewed Apr 28, 2023

View reviewed changes

build.gradle Outdated Show resolved Hide resolved

sreuland reviewed Apr 28, 2023

View reviewed changes

Ifropc added 2 commits April 28, 2023 14:07

Replace Java Optional with Guava

5487a36

Use Integer

4033124

sreuland reviewed Apr 28, 2023

View reviewed changes

sreuland approved these changes Apr 28, 2023

View reviewed changes

Optimize imports

22d8d55

Ifropc merged commit 9444976 into lightsail-network:master May 1, 2023

mollykarcher mentioned this pull request May 1, 2023

servics/horizon: send Retry-After header in rate-limited responses stellar/go#4858

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid to have unexpected exception on status code 429 TooManyRequests #433

Avoid to have unexpected exception on status code 429 TooManyRequests #433

vinamogit commented May 22, 2022

leighmcculloch left a comment

leighmcculloch Apr 28, 2023

leighmcculloch Apr 28, 2023 •

edited

Loading

Ifropc Apr 28, 2023

leighmcculloch Apr 28, 2023

sreuland Apr 28, 2023 •

edited

Loading

leighmcculloch Apr 28, 2023

Ifropc Apr 28, 2023

sreuland Apr 28, 2023

leighmcculloch Apr 28, 2023 •

edited

Loading

mollykarcher Apr 28, 2023

sreuland Apr 28, 2023

Ifropc Apr 28, 2023

sreuland Apr 28, 2023

sreuland Apr 28, 2023

sreuland Apr 28, 2023

sreuland left a comment

		@@ -1,5 +1,6 @@
		package org.stellar.sdk.requests;

		import com.google.common.base.Optional;

Avoid to have unexpected exception on status code 429 TooManyRequests #433

Avoid to have unexpected exception on status code 429 TooManyRequests #433

Conversation

vinamogit commented May 22, 2022

leighmcculloch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leighmcculloch Apr 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sreuland Apr 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leighmcculloch Apr 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sreuland left a comment

Choose a reason for hiding this comment

leighmcculloch Apr 28, 2023 •

edited

Loading

sreuland Apr 28, 2023 •

edited

Loading

leighmcculloch Apr 28, 2023 •

edited

Loading