NPE in SimpleClient-HttpURLConnection with errorstream-buffering [SPR-16773]

[spring-projects-issues]

Markus opened SPR-16773 and commented

For an Jersey-Client we enabled errorstream-buffering in HttpUrlConnection. Due to global properties this also affected Spring-SimpleClientHttpRequestFactory and subsequently caused NullPointerException for httpstatus-errors. Seems to happen everytime for 4XX or 5XX with non-empty body.

It seems that Java8-HttpUrlConnection doesnt expect when buffered errorstream is attempted to drain after already closed. The drain-after-close occurs because DefaultResponseErrorHandler reads and closes the response-inputstream, and a later finally-block in RestTemplate#doExecute drains+closes again.

 

java.lang.NullPointerException
 at sun.net.www.protocol.http.HttpURLConnection$ErrorStream.read(HttpURLConnection.java:3690)
 at sun.net.www.protocol.http.HttpURLConnection$ErrorStream.read(HttpURLConnection.java:3685)
 at org.springframework.util.StreamUtils.drain(StreamUtils.java:201)
 at org.springframework.http.client.SimpleClientHttpResponse.close(SimpleClientHttpResponse.java:103)
 at org.springframework.web.client.RestTemplate.doExecute(RestTemplate.java:747)
 at org.springframework.web.client.RestTemplate.execute(RestTemplate.java:686)
 

 

I will put up a MergeRequest shortly

---

Affects: 4.3.16, 5.0.5

Reference URL: #1810

Issue Links:

• HTTP persistent connections for HTTP Invoker and RestTemplate [SPR-14040] #18612 HTTP persistent connections for HTTP Invoker and RestTemplate

Backported to: 4.3.17

[spring-projects-issues]

Markus commented

MergeRequest: #1810

[spring-projects-issues]

Juergen Hoeller commented

I ended up going with the simplest possible solution, not least of it all for a side-effect-free backport to 4.3.x: extending the catch(IOException) block to catch(Exception) in SimpleClientHttpResponse.close()... defensively handling any cleanup exceptions. Arguably the NPE coming out of HttpURLConnection.ErrorStream is a bug in its own right and should really be an IOException or at least an IllegalStateException, generally suggesting such a defensive catch block.{{}}