http-netty: let RetryingHttpRequesterFilter return responses on failure #3048

bryce-anderson · 2024-08-22T23:44:41Z

Motivation:

Sometimes people just want to get the last failed response when the retry
loop ends. However, right now we only yield the exceptions that where
created. Users can't do this smuggling themselves in a generic way via the
HttpResponseException because it could lead to resource leaks.

Modifications:

Let users simply return the last failed response when the retry loop
exits unsuccessfully.

Motivation: Sometimes people just want to get the last failed response when the retry loop ends. However, right now we only yield the exceptions that where created. Users can't do this smuggling themselves in a generic way via the HttpResponseException because it could lead to resource leaks. Modifications: Let users simply return the last failed response when the retry loop exits unsuccessfully.

bryce-anderson · 2024-08-22T23:52:04Z

servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/RetryingHttpRequesterFilter.java

+        public Builder returnFailedResponses(final boolean returnFailedResponses) {
+            this.returnFailedResponses = returnFailedResponses;
+            return this;
+        }


I'm certain this can have a better name and clearly it needs docs before merging. Name suggestions welcome.

I also think this API is a bit awkward: first you must turn a response into an HttpResponseException and then it's going to be discarded. Alternatively, we could just have a different lambda to the tune of Function<Boolean, HttpResponseMetadata> shouldRetry.

Right now we don't have RS operators to achieve retries without mapping into exceptions. If we go the route of clean retry of response meta-data without mapping to exceptions, it's possible but will take longer.

Current rational was that some users want to always map responses to exceptions, that's why we have independent responseMapper. Then some users may want to retry that, so there is a 2nd method for them to retryResponses. We decided to put them next to each other on the same builder instead of offering 2 different filters bcz they often used together.

I agree that having a 3rd method that works only if the other 2 also configured is not intuitive. Alternatively, we can consider adding a retryResponses overload that takes a boolean to make a decision if it need to unwrap the original response or not.

I like the idea of the boolean overload, which would signal that it needs to be configured "together". Alternatively when building, we should at least check if this value is set to true and others are in their default state to reject the config?

servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/RetryingHttpRequesterFilter.java

bryce-anderson · 2024-08-23T15:22:27Z

A risk of this PR is that it's very difficult to know for sure that the deferred response is properly drained since it gets smuggled through the error channel.

idelpivnitskiy · 2024-10-01T11:25:16Z

servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/RetryingHttpRequesterFilter.java

+        public Builder returnFailedResponses(final boolean returnFailedResponses) {
+            this.returnFailedResponses = returnFailedResponses;
+            return this;
+        }


Right now we don't have RS operators to achieve retries without mapping into exceptions. If we go the route of clean retry of response meta-data without mapping to exceptions, it's possible but will take longer.

Current rational was that some users want to always map responses to exceptions, that's why we have independent responseMapper. Then some users may want to retry that, so there is a 2nd method for them to retryResponses. We decided to put them next to each other on the same builder instead of offering 2 different filters bcz they often used together.

I agree that having a 3rd method that works only if the other 2 also configured is not intuitive. Alternatively, we can consider adding a retryResponses overload that takes a boolean to make a decision if it need to unwrap the original response or not.

servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/RetryingHttpRequesterFilter.java

…psonses

bryce-anderson · 2024-10-08T20:56:46Z

@idelpivnitskiy, with the additional constraint of not returning the body this got dramatically simpler but I'm not certain having an empty response body is what we wanted.

…psonses

daschl · 2024-11-11T08:34:54Z

servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/RetryingHttpRequesterFilter.java

+                    result = result.onErrorMap(backoffError -> ThrowableUtils.addSuppressed(t, backoffError))
+                            // If we get cancelled we also need to drain the message body as there is no guarantee
+                            // we'll ever receive a completion event, error or success.
+                            .beforeCancel(() -> drain(response).subscribe())


does that retry draining collide/overlap with the draining @idelpivnitskiy added in the other PR?

I don't think so. That leak should have been self contained and the problem was that we didn't drain a response that the redirect filter had decided to consume itself.

servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/RetryingHttpRequesterFilter.java

daschl · 2024-11-11T08:43:03Z

servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/RetryingHttpRequesterFilter.java

+        public Builder returnFailedResponses(final boolean returnFailedResponses) {
+            this.returnFailedResponses = returnFailedResponses;
+            return this;
+        }


I like the idea of the boolean overload, which would signal that it needs to be configured "together". Alternatively when building, we should at least check if this value is set to true and others are in their default state to reject the config?

…psonses

daschl

LGTM overall, just some minor comments on docs

servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/RetryingHttpRequesterFilter.java

...talk-http-netty/src/test/java/io/servicetalk/http/netty/RetryingHttpRequesterFilterTest.java

…ttpRequesterCanReturnRepsonses

idelpivnitskiy

Sorry that it took so long to review. I like the approach, just some comments to make it stronger:

idelpivnitskiy · 2024-12-20T21:18:33Z

...talk-http-netty/src/test/java/io/servicetalk/http/netty/RetryingHttpRequesterFilterTest.java

                        // Disable request retrying
                        .retryRetryableExceptions((requestMetaData, e) -> ofNoRetries())
                        // Retry only responses marked so
-                        .retryResponses((requestMetaData, throwable) -> ofImmediate(maxTotalRetries - 1))
+                        .retryResponses((requestMetaData, throwable) -> {
+                            if (throwable instanceof HttpResponseException &&


Is it some intermediate change that not needed anymore? I tried locally reverting back to the original lambda and it worked.

idelpivnitskiy · 2024-12-20T21:19:54Z

...talk-http-netty/src/test/java/io/servicetalk/http/netty/RetryingHttpRequesterFilterTest.java

-        assertThat("Unexpected exception.", e, instanceOf(HttpResponseException.class));
+        if (returnFailedResponses) {
+            HttpResponse response = normalClient.request(normalClient.get("/"));
+            assertThat(response.status(), is(HttpResponseStatus.OK));


Consider enhancing server response to also add payload body and asserting here that payload is not drained.

idelpivnitskiy · 2024-12-20T21:28:52Z

servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/RetryingHttpRequesterFilter.java

@@ -798,9 +844,28 @@ public Builder maxTotalRetries(final int maxRetries) {
         * @param mapper a {@link Function} that maps a {@link HttpResponseMetaData} to an
         * {@link HttpResponseException} or returns {@code null} if there is no mapping for response meta-data. The
         * mapper should return {@code null} if no retry is needed or if it cannot be determined that a retry is needed.


Feels like this "The mapper should return" statement is here by mistake bcz this mapper is for mapping only, retries are handled by retryResponses. While you are here, could you please clean it up on both overloads?

idelpivnitskiy · 2024-12-20T21:30:54Z

servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/RetryingHttpRequesterFilter.java

+         * @return {@code this}
+         */
+        public Builder responseMapper(final Function<HttpResponseMetaData, HttpResponseException> mapper,
+                                      final boolean returnFailedResponses) {


To me it feels like this boolean better belongs to retryResponses rather than responseMapper bcz users may decide to map 4xx/5xx to exceptions even if they are no going to retry them. WDYT?

idelpivnitskiy · 2024-12-20T22:05:39Z

servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/RetryingHttpRequesterFilter.java

+                        // If we succeed, we need to drain the response body before we continue. If we fail we want to
+                        // surface the original exception and don't worry about draining since it will be returned to
+                        // the user.
+                        result = result.onErrorMap(backoffError -> ThrowableUtils.addSuppressed(t, backoffError))


In what cases is it possible that backoffError != t? Do we really need this mapping?

idelpivnitskiy · 2024-12-20T22:06:46Z

servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/RetryingHttpRequesterFilter.java

+                        // the user.
+                        result = result.onErrorMap(backoffError -> ThrowableUtils.addSuppressed(t, backoffError))
+                                // If we get cancelled we also need to drain the message body as there is no guarantee
+                                // we'll ever receive a completion event, error or success.


Thanks for clarification! Consider adding: and it's legit to do that because subscriber is no longer interested in the response

idelpivnitskiy · 2024-12-20T22:09:16Z

servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/RetryingHttpRequesterFilter.java

+                    } else if (LOGGER.isDebugEnabled()) {
+                        if (!(t instanceof HttpResponseException)) {
+                            LOGGER.debug("Couldn't unpack response due to unexpected dynamic types. Required " +
+                                    "exception of type HttpResponseException, found {}", t.getClass());


Do we need to log in case t is not HttpResponseException? Any other exception type will fall under this condition and produce logging. Users may come to us worried about it.

+1 for logging when the exception type is HttpResponseException but metaData is not StreamingHttpResponse. It might be even better to increase this to info/warn and add an assertion.
Should we log here or inside single.onErrorResume(HttpResponseException.class, ...) or in both places?

idelpivnitskiy · 2024-12-20T22:17:06Z

servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/RetryingHttpRequesterFilter.java

+         * @param mapper a {@link Function} that maps a {@link HttpResponseMetaData} to an
+         * {@link HttpResponseException} or returns {@code null} if there is no mapping for response meta-data. The
+         * mapper should return {@code null} if no retry is needed or if it cannot be determined that a retry is needed.
+         * @param returnFailedResponses whether to unwrap the response defined by the {@link HttpResponseException}


Wdyt about naming it returnOriginalResponses?

idelpivnitskiy · 2024-12-20T22:24:27Z

servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/RetryingHttpRequesterFilter.java

@@ -258,19 +291,31 @@ protected Single<StreamingHttpResponse> request(final StreamingHttpRequester del
            if (responseMapper != null) {
                single = single.flatMap(resp -> {
                    final HttpResponseException exception = responseMapper.apply(resp);


Unrelated to your changes, but a find: if user-defined responseMapper throws, we leak resp 😢

idelpivnitskiy · 2024-12-20T22:30:30Z

servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/RetryingHttpRequesterFilter.java

@@ -109,13 +117,14 @@ public final class RetryingHttpRequesterFilter

    RetryingHttpRequesterFilter(
            final boolean waitForLb, final boolean ignoreSdErrors, final boolean mayReplayRequestPayload,
-            final int maxTotalRetries,
+            final boolean returnFailedResponses, final int maxTotalRetries,
            @Nullable final Function<HttpResponseMetaData, HttpResponseException> responseMapper,
            final BiFunction<HttpRequestMetaData, Throwable, BackOffPolicy> retryFor,


It seems like we may leak "pending response" if any user-defined function under retryFor throws. Scenario:

Server responds with 503

User maps it to HttpResponseException

returnFailedResponses == true, we don't drain proactively

OuterRetryStrategy.apply is invoked

retryFor throws

We won't invoke applyRetryCallbacks and won't drain that response.

Consider adding a try-catch inside apply to make sure we drain t instance of HttpResponseException in case of any unexpected exception.

Adding a test will be highly appreciated.

bryce-anderson added 2 commits August 22, 2024 17:42

Add a test

5b7f014

bryce-anderson requested review from tkountis and idelpivnitskiy August 22, 2024 23:44

make linters happy

b08742d

bryce-anderson commented Aug 22, 2024

View reviewed changes

servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/RetryingHttpRequesterFilter.java Outdated Show resolved Hide resolved

idelpivnitskiy reviewed Oct 1, 2024

View reviewed changes

bryce-anderson added 2 commits October 8, 2024 12:01

Merge branch 'main' into bl_anderson/RetryingHttpRequesterCanReturnRe…

4b4b60c

…psonses

Some of idels feedback

3493dc4

bryce-anderson force-pushed the bl_anderson/RetryingHttpRequesterCanReturnRepsonses branch from c751f7c to 840fab0 Compare October 8, 2024 20:35

Remove the second wrapper

e80e98e

bryce-anderson force-pushed the bl_anderson/RetryingHttpRequesterCanReturnRepsonses branch from 840fab0 to e80e98e Compare October 8, 2024 20:47

bryce-anderson requested a review from idelpivnitskiy October 8, 2024 20:55

Merge branch 'main' into bl_anderson/RetryingHttpRequesterCanReturnRe…

6ba45bc

…psonses

bryce-anderson marked this pull request as ready for review October 11, 2024 15:29

bryce-anderson requested a review from daschl October 11, 2024 15:29

bryce-anderson marked this pull request as draft October 21, 2024 21:28

bryce-anderson added 4 commits October 28, 2024 11:43

Merge branch 'main' into bl_anderson/RetryingHttpRequesterCanReturnRe…

6b45aa8

…psonses

Keep the response body

49d272d

Merge branch 'main' into bl_anderson/RetryingHttpRequesterCanReturnRe…

f25458c

…psonses

Make sure we drain the response if the backoff Completable is cancelled

8d07708

bryce-anderson marked this pull request as ready for review November 4, 2024 22:21

daschl reviewed Nov 11, 2024

View reviewed changes

bryce-anderson added 3 commits December 2, 2024 10:51

Merge branch 'main' into bl_anderson/RetryingHttpRequesterCanReturnRe…

3594f3a

…psonses

Feedback

0a47202

Even better log messages

ea561aa

bryce-anderson requested a review from daschl December 3, 2024 22:08

daschl approved these changes Dec 17, 2024

View reviewed changes

bryce-anderson added 2 commits December 20, 2024 12:01

Merge remote-tracking branch 'origin/main' into bl_anderson/RetryingH…

a4d023c

…ttpRequesterCanReturnRepsonses

Michaels feedback

428a2d1

idelpivnitskiy reviewed Dec 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http-netty: let RetryingHttpRequesterFilter return responses on failure #3048

http-netty: let RetryingHttpRequesterFilter return responses on failure #3048

bryce-anderson commented Aug 22, 2024

bryce-anderson Aug 22, 2024

bryce-anderson Aug 23, 2024

idelpivnitskiy Oct 1, 2024

daschl Nov 11, 2024

bryce-anderson commented Aug 23, 2024

idelpivnitskiy Oct 1, 2024

bryce-anderson commented Oct 8, 2024

daschl Nov 11, 2024

bryce-anderson Dec 2, 2024

daschl Nov 11, 2024

daschl left a comment

idelpivnitskiy left a comment

idelpivnitskiy Dec 20, 2024

idelpivnitskiy Dec 20, 2024

idelpivnitskiy Dec 20, 2024

idelpivnitskiy Dec 20, 2024

idelpivnitskiy Dec 20, 2024

idelpivnitskiy Dec 20, 2024

idelpivnitskiy Dec 20, 2024

idelpivnitskiy Dec 20, 2024

idelpivnitskiy Dec 20, 2024

idelpivnitskiy Dec 20, 2024 •

edited

Loading

http-netty: let RetryingHttpRequesterFilter return responses on failure #3048

Are you sure you want to change the base?

http-netty: let RetryingHttpRequesterFilter return responses on failure #3048

Conversation

bryce-anderson commented Aug 22, 2024

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

bryce-anderson commented Aug 23, 2024

Choose a reason for hiding this comment

bryce-anderson commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschl left a comment

Choose a reason for hiding this comment

idelpivnitskiy left a 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

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

idelpivnitskiy Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

idelpivnitskiy Dec 20, 2024 •

edited

Loading