Fixes an issue that can be triggered in reactive WebClients when the server throws a malformed response chunk.

I picked up on the original issue by using WireMock with the following test case:

        WireMockServer server = new WireMockServer(SocketUtils.findAvailableTcpPort());
        server.start();
        WebClient client = WebClient
            .builder()
            .baseUrl(server.baseUrl())
            .clientConnector(new ReactorClientHttpConnector(HttpClient.create().wiretap(true)))
            .build();

        server.givenThat(post("/").willReturn(
            aResponse()
                .withFault(Fault.MALFORMED_RESPONSE_CHUNK)
                .withHeader("Response-Header-1", "value 1")
                .withHeader("Response-Header-2", "value 2")
                .withBody("{\"message\": \"Hello, World!\"}"))
        );

        Mono<?> result = client
            .post()
            .retrieve()
            .toBodilessEntity();

        assertThrows(WebClientException.class, result::block);

...where we see an unhandled exception get thrown. 

org.opentest4j.AssertionFailedError: Unexpected exception type thrown ==> expected: <org.springframework.web.reactive.function.client.WebClientException> but was: <reactor.core.Exceptions.ReactiveException>
    at org.junit.jupiter.api.AssertThrows.assertThrows(AssertThrows.java:65)
    at org.junit.jupiter.api.AssertThrows.assertThrows(AssertThrows.java:37)
    at org.junit.jupiter.api.Assertions.assertThrows(Assertions.java:3007)
        ....
Caused by: reactor.core.Exceptions$ReactiveException: reactor.netty.http.client.PrematureCloseException: Connection prematurely closed DURING response
    at reactor.core.Exceptions.propagate(Exceptions.java:392)
    at reactor.core.publisher.BlockingSingleSubscriber.blockingGet(BlockingSingleSubscriber.java:97)
    at reactor.core.publisher.Mono.block(Mono.java:1704)
    at org.junit.jupiter.api.AssertThrows.assertThrows(AssertThrows.java:55)
    ... 71 more
    Suppressed: java.lang.Exception: #block terminated with an error
    at reactor.core.publisher.BlockingSingleSubscriber.blockingGet(BlockingSingleSubscriber.java:99)
        ... 73 more
Caused by: reactor.netty.http.client.PrematureCloseException: Connection prematurely closed DURING response
    Suppressed: reactor.core.publisher.FluxOnAssembly$OnAssemblyException: 
Error has been observed at the following site(s):
    |_ checkpoint ⇢ Body from POST http://localhost:23337 [DefaultClientResponse]
Stack trace:

(With no additional stacktrace provided by the underlying reactor library).

This exception does not appear to be documented in the most recent documentation for this codepath, and since the various HTTP client connectors all use their own variants for these exceptions, it makes it overly difficult for developers to add suitable error handling for components such as in-house libraries for projects relying on Spring WebFlux without hardcoding edge cases for all libraries by default. Therefore, my assumption has been that this is a potential bug as a result.

The issue itself appears to be being caused by the body processing throwing the exception rather than the call to exchange() itself. On bodiless entities this appears to defer to be lazy, and the reactor client itself also appears to be only reporting the malformed chunks after the body has been processed, which meant that the existing error handling in ExchangeFunctions was not being hit for these cases.

For some reason, the OKHTTP3 Mock Web Server is not chunking responses properly in such a way that I could simulate this in integration tests, so I resorted to writing a simple thread with a ServerSocket to simulate the same issue. If anyone can think of a nicer way of achieving this, I will be happy to replace it, but for now it appears to cover that test case.

I have also expanded the new test pack that was added on the 8th for WebClient integration tests to cover a couple of other test cases that I was performing on an in-house project in the company I work for that also appear to not have existing cases yet. These are handling scenarios where the server closes the socket before the response has been sent or midway through the request being received. These cases passed already but I added them just to be certain that no other regressions were added in this error handling flow.

I am not aware of any issues that cover this already, but I may have missed some. I did take the time to try and find any that may reference something related to this, but if I have missed anything I will update the PR accordingly.

Hope what I have added is all okay, as I have only ever contributed documentation changes before now. If there is any feedback or anyone can think of a better way to resolve this; or if there is any reason for this behavior needing to be left how it is, please let me know!

Comment From: rstoyanchev

@ascopes thanks for the detailed report and pull request.

Consistently emitting exceptions that extend from WebClientException was a goal for 5.3 as explained in https://github.com/spring-projects/spring-framework/issues/23842#issuecomment-555447998 and DefaultWebClient does detect the disconnect but the DefaultClientResponse#createException method tries to read the body and predictably fails, but from there it seems to only handle IllegalStateException and not disconnect errors.

I think we need to expand the handling of errors in DefaultClientResponse#createException. I'm not entirely sure why we only catch IllegalStateException. It seems like any error we run into at that point, should result in a WebClientResponseException with an empty body. @poutsma what's your take on this?

In the mean time, @ascopes, could you please separate out the additional test cases that are unrelated to the fix into a separate PR, which I should be able to process independently. Thanks again.

Comment From: poutsma

It seems like any error we run into at that point, should result in a WebClientResponseException with an empty body. @poutsma what's your take on this?

I agree. I've tried to recollect, but can't remember why it is only triggered for IllegalStateExceptions.

On the specific topic of the reactor.core.Exceptions.ReactiveException, I would argue that this connector-specific exception should be converted in the ReactorClientHttpConnector (and its related classes ReactorClientHttpRequest and ReactorClientHttpResponse).

Comment From: ascopes

@rstoyanchev can do, it will most likely be some time next week before I will have time to look into this though, if that is okay.

Reason these were bundled in here was because I was attempting to verify if any other closure-related scenarios existed with the same issue.

On the topic of exceptions, would it perhaps be worth considering having a third type of web application exception subclass specifically for IO-based errors below the codec level? Since these usually would be expected to be error scenarios anyway, this may simplify error handling by not having to differentiate between an empty response with a cause in-code.

I believe that the RestTemplate API does something very similar to this conceptually.

Comment From: rstoyanchev

I've incorporated the commit with the tests, then simplified a little, removed the unrelated test, and applied a fix. That should take care of the main issue.