sbordet
commented
4 months ago HttpClient event listeners implementations are not supposed to throw.
In this example, HttpClient's Response.HeadersListener calls the implementation in the reactive client, which calls Spring, so Spring should not throw.
From the point of view of HttpClient the event was delivered, but it cannot know if the listener implementation did enough to continue with the processing of the request/response or not.
That's why it was decided to catch and log rather than catch and abort.
Consider a valid response (e.g. with a valid status code), and listener implementation such as:
public void onHeaders(Response response) {
System.err.println("HEADERS");
throw new NullPointerException();
}From the point of view of HttpClient the response is still valid; HttpClient can read and notify content events until the whole response is properly processed. The fact that an application listener threw does not affect, in general, the processing of the response.
It was deemed too harsh to abort, because the response has arrived, it is valid, and can be processed further.
There is an explicit API to abort a request or a response, so applications should use that.
What we can do is to be more aggressive at parsing a response with a status code < 100 or >= 600, as per RFC 9110, and generate a parser error.
In this way, HttpClient would catch the bad message earlier, fail early, and Spring would not fail (because it would not be called for the "headers" event, but only for a "bad response" event).
@gregw WDYT? Should we be more strict at parsing status codes?
However, in general, if an event listener implementation can throw, it should catch and decide what to do: sometimes the error is recoverable, other times it is fatal and the abort APIs should be used.
joakime
Jetty version(s) Jetty
11.0.20Jetty ReactiveStream HttpClient1.1.13Java version/vendor
(use: java -version)openjdk17.0.10OS type/version Windows
Description If an http response contains an invalid status code <100, an
IllegalArgumentExceptionis thrown by Spring'sHttpStatusCodeinterface, when theResponseNotifiertries to execute theonHeadersmethod. TheonHeadersmethod of Jetty ReactiveStream Client'sResponseListenerProcessorclass, which is one of the registered listeners that are notified, applies the content function, which calls the constructor ofJettyClientHttpResponse. The problem only surfaced after this commit was introduce to Spring, which introduced aHttpStatusCode.valueOfmethod call in theJettyClientHttpResponseconstructor to create a status code object, which then throws the exception mentioned above.The problem is, that the
ResponseNotifiercatches all exceptions, logs them and doesn't terminate the exchange nor propagates the exception to the caller. Therefore http calls with status codes <100 run into a timeout resulting in aTimeoutException, with no indication of the real cause.The following Exception is logged (see log excerpt for further details incl. timeout exception):
How to reproduce?
``` import java.time.Duration; import org.eclipse.jetty.client.HttpClient; import org.eclipse.jetty.client.http.HttpClientTransportOverHTTP; import org.springframework.boot.SpringApplication; import org.springframework.boot.autoconfigure.SpringBootApplication; import org.springframework.http.ResponseEntity; import org.springframework.http.client.reactive.JettyClientHttpConnector; import org.springframework.web.reactive.function.client.WebClient; import okhttp3.mockwebserver.MockResponse; import okhttp3.mockwebserver.MockWebServer; @SpringBootApplication public class JettyBugReproductionExampleApplication { public static void main(String[] args) throws Exception { SpringApplication.run(JettyBugReproductionExampleApplication.class, args); HttpClient httpClient = new HttpClient(new HttpClientTransportOverHTTP()); JettyClientHttpConnector jettyClientHttpConnector = new JettyClientHttpConnector(httpClient); WebClient webClient = WebClient.builder() .clientConnector(jettyClientHttpConnector) .build(); MockWebServer mockWebServer = new MockWebServer(); mockWebServer.start(1337); MockResponse mockResponse = new MockResponse().setResponseCode(50); mockWebServer.enqueue(mockResponse); try { ResponseEntityLog excerpt
``` 2024-05-03 15:20:09,953 DEBUG - [org.eclipse.jetty.client.HttpSender] - Request headers HttpRequest[GET / HTTP/1.1]@62735b13 Accept-Encoding: gzip User-Agent: Jetty/11.0.20 Accept: */* Host: localhost:1337 2024-05-03 15:20:09,953 DEBUG - [org.eclipse.jetty.client.util.AbstractRequestContent] - Content demand, producing true for BytesRequestContent.SubscriptionImpl@1f39be57[demand=1,stalled=false,committed=false,emitInitial=true] 2024-05-03 15:20:09,954 DEBUG - [org.eclipse.jetty.client.util.AbstractRequestContent] - Notifying content last=true HeapByteBuffer@12c7d102[p=0,l=0,c=0,r=0]={<<<>>>} for BytesRequestContent.SubscriptionImpl@1f39be57[demand=0,stalled=false,committed=true,emitInitial=true] 2024-05-03 15:20:09,954 DEBUG - [org.eclipse.jetty.client.HttpSender] - Content HeapByteBuffer@12c7d102[p=0,l=0,c=0,r=0]={<<<>>>} last=true for HttpRequest[GET / HTTP/1.1]@62735b13 2024-05-03 15:20:09,955 DEBUG - [org.eclipse.jetty.client.http.HttpSenderOverHTTP] - Sending headers with content HeapByteBuffer@12c7d102[p=0,l=0,c=0,r=0]={<<<>>>} last=true for HttpRequest[GET / HTTP/1.1]@62735b13 2024-05-03 15:20:09,956 DEBUG - [org.eclipse.jetty.client.http.HttpSenderOverHTTP] - Generated headers (-1 bytes), chunk (-1 bytes), content (0 bytes) - NEED_HEADER/HttpGenerator@4dad3475{s=START} for HttpRequest[GET / HTTP/1.1]@62735b13 2024-05-03 15:20:09,957 DEBUG - [org.eclipse.jetty.http.HttpGenerator] - generateHeaders GET{u=/,HTTP/1.1,h=4,cl=0,p=null} last=true content=HeapByteBuffer@12c7d102[p=0,l=0,c=0,r=0]={<<<>>>} 2024-05-03 15:20:09,957 DEBUG - [org.eclipse.jetty.http.HttpGenerator] - Accept-Encoding: gzip User-Agent: Jetty/11.0.20 Accept: */* Host: localhost:1337 2024-05-03 15:20:09,957 DEBUG - [org.eclipse.jetty.http.HttpGenerator] - NO_CONTENT 2024-05-03 15:20:09,957 DEBUG - [org.eclipse.jetty.client.http.HttpSenderOverHTTP] - Generated headers (103 bytes), chunk (-1 bytes), content (0 bytes) - FLUSH/HttpGenerator@4dad3475{s=COMPLETING} for HttpRequest[GET / HTTP/1.1]@62735b13 2024-05-03 15:20:09,957 DEBUG - [org.eclipse.jetty.io.WriteFlusher] - write: WriteFlusher@2e9619a6{IDLE}->null [DirectByteBuffer@7535878b[p=0,l=103,c=4096,r=103]={<<