RestTemplate reading Json prohibits JDK HttpClient connection reuse (keep-alive)

[apinske]

Affects: 5.3.14

There is an issue regarding https connection reuse (keep-alive) when using jackson unmarshaller, chunked transfer encoding and RestTemplate. The connections are not reused, which leads to more TLS handshakes for subsequent calls.

• When the Jackson AUTO_CLOSE_SOURCE-Feature is enabled (which is the default), jackson closes the HttpInputStream in UTF8StreamJsonParser._closeInput() after the json doc is fully read.
• The stream is not at EOF yet, though. The last-chunk (as of RFC7230 4.1) is still "waiting on the wire".
• This leads to a call to sun.net.www.http.ChunkedInputStream.hurry() which aims to read all the remaining bytes.
• This does not succeed fully, because in sun.net.www.http.ChunkedInputStream.readAheadNonBlocking() in.available() may not return all remaining bytes.
• This can lead to a ChunkedInputStream that is not in state STATE_DONE, which will lead to it being closed, instead of kept alive.

This is a combination of multiple unrelated things, that may not be issues in their own rights.

• JDK-HttpClient hurry() may leave unread bytes in the stream, which is probably okay, as close() was called prematurely.
• Jackson by default calls close() prematurely, having not read all input.
• Spring provides Jackson in its default configuration on top of JDK-HttpClient.
  • Spring has its own feature to drain http streams, and should probably not rely on Jackson closing the stream.

I open this bug against Spring, because I think it is the best place to fix the issue.

Find a reproducing project at: https://github.com/apinske/playground-http/tree/spring-framework-issues-27969

[rstoyanchev]

There was a commit a while ago b947bfe along these lines, ensuring the response stream is drained first when ClientHttpResponse#close() is called. Maybe we need to do the same for the close on the InputStream itself.

What do you think @bclozel?

[bclozel]

Right now I'm missing the difference between this use case and the one that was fixed in b947bfe.

The sample project calls restTemplate.getForObject which internally calls doExecute and closes the response. So the response body should be in fact drained. Would draining as soon as the input stream is closed change the behavior here?

Note that I have yet to run the sample locally, so this comment is probably missing a key fact.

[apinske]

The test in b947bfe does not use jackson, which closes the stream before the spring mechanism (drain+close) can do it. The subsequent IOException (during spring's drain attempt) is swallowed.

[rstoyanchev]

Team decision: Recently we reviewed a request #27773 to ensure the InputStream is closed but decided that converters do not have enough context to do that. Here we'll ensure that they consistently don't close the InputStream by using StreamUtils#nonClosing. This is something that was done a long time ago in 6661788 but not applied the the Jackson converter at the time. Later, the writing side of Jackson was included 7be7e5b, but not for reading.