Emit WebClientResponseException for malformed HTTP response

[ascopes]

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!

[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 #23842 (comment) 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.

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

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

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