http-netty: let RetryingHttpRequesterFilter return responses on failure #3048
Conversation
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.
| public Builder returnFailedResponses(final boolean returnFailedResponses) { | ||
| this.returnFailedResponses = returnFailedResponses; | ||
| return this; | ||
| } |
There was a problem hiding this comment.
I'm certain this can have a better name and clearly it needs docs before merging. Name suggestions welcome.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
| // If we're retrying due to a wrapped response it's because the users want the actual response, | ||
| // not an exception. Therefore, we return the wrapped response and let it get unwrapped at the | ||
| // end of the retry pipeline. | ||
| subscriber.onError(t instanceof WrappedResponseException ? t : tt); |
There was a problem hiding this comment.
This pathway needs a test.
|
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. |
| public Builder returnFailedResponses(final boolean returnFailedResponses) { | ||
| this.returnFailedResponses = returnFailedResponses; | ||
| return this; | ||
| } |
There was a problem hiding this comment.
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
Outdated
Show resolved
Hide resolved
| return Single.succeeded(resp).shareContextOnSubscribe(); | ||
| } | ||
| if (returnFailedResponses) { | ||
| return Single.failed(new WrappedResponseException(resp, exception)); |
There was a problem hiding this comment.
We don't need another wrapper over the existing HttpResponseException. Consider unwrapping HttpResponseException inside the final onErrorResume.
servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/RetryingHttpRequesterFilter.java
Outdated
Show resolved
Hide resolved
bryce-anderson
force-pushed
the
bl_anderson/RetryingHttpRequesterCanReturnRepsonses
branch
from
c751f7c
to
840fab0
Compare
bryce-anderson
force-pushed
the
bl_anderson/RetryingHttpRequesterCanReturnRepsonses
branch
from
840fab0
to
e80e98e
Compare
|
@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. |
| 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()) |
There was a problem hiding this comment.
does that retry draining collide/overlap with the draining @idelpivnitskiy added in the other PR?
| completable.beforeOnComplete(() -> retryCallbacks.beforeRetry(retryCount, requestMetaData, t)); | ||
| Completable result = (retryCallbacks == null ? completable : | ||
| completable.beforeOnComplete(() -> retryCallbacks.beforeRetry(retryCount, requestMetaData, t))); | ||
| if (returnFailedResponses && t instanceof HttpResponseException && |
There was a problem hiding this comment.
should there at least be debug level warning if the types are not what we expect since then the block is not executed?
| public Builder returnFailedResponses(final boolean returnFailedResponses) { | ||
| this.returnFailedResponses = returnFailedResponses; | ||
| return this; | ||
| } |
There was a problem hiding this comment.
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?
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.