Fix response leak that can be caused by an exception during redirect

[idelpivnitskiy]

Motivation:

RedirectConfig accepts user-defined functions and it's possible that some of them can throw. RedirectSingle expects it and applies a try-catch block, but we forgot to drain the response payload body before propagating the error.

Modifications:

• Drain response payload before propagating an exception during redirect processing;
• Enhance tests to validate expected behavior;

Result:

We do not leave undrained response payload if redirect fails with an exception.

[bryce-anderson]

In the terminalDelivered == true path we still don't drain and I see that we set that flag to true before we call many of the callbacks, eg

terminalDelivered = true;
target.onSuccess(response);

If it is target.onSuccess(..) that threw we will potentially still leak unless the onSuccess call did the draining. That does seem like it should be the responsibility of the onSucess method, but how defensive do we want to be?

[idelpivnitskiy]

The logic here is that we set the terminalDelivered = true and in the next line we subscribe to nextResponse single, which will first subscribe to response.messageBody().ignoreElements(), register its cancellable in sequentialCancellable, and only then will deal with target. This guarantees that we do not leak the response, it will either terminate or get canceled.

[bryce-anderson]

I think we may be talking about different cases as I'm talking about those higher up which you didn't modify. For example:

                final String location = redirectLocation(redirectCount, request, response);
                if (location == null) {
                    terminalDelivered = true;
                    target.onSuccess(response); // <- if I throw, is the response body drained or not?
                    return;
                }

There are a few others that follow the same pattern. Trying to determine if the body has been drained is perhaps too defensive but we also don't normally expect onSuccess(..) to throw so maybe it's better to aggressively drain?

[daschl]

Change otherwise LGTM, but I'd also like to understand how the flow works in the case that @bryce-anderson outlined.

[idelpivnitskiy]

Good point, if users have an unexpected exception inside an operator applied later (for example, Single.map(...)), there is a risk they can leak the response as well.
First, users code is outside of our responsibility. They should not throw from operators but propagate cancel/onError via reactive flow.
Second, as we discussed offline we can provide a general filter for all clients to handle this type of problem with the best effort. I will open a follow-up for that.

Have a new question

[bryce-anderson]

I'm still curious what others think about draining if the target.onSuccess(..) calls throw, but that is subjective and this patch fixes an objective leak so I'm happy to 🚢 as is.

[Scottmitch]

I think I recall asking this previously (sorry if dup-question), but what if the payload body is very large or doesn't complete (malicious client, networking broken, etc.)? Will our timeouts kick-in at this level?

[idelpivnitskiy]

It depends on how users configure timeouts for their use cases. If they have it at the response payload body level, then yes.
We can add more protection as a separate work item and unify all places, bcz this is not the only place that drains payload.

[Scottmitch]

followup PR sgtm