Hi,
Our project was reviewing a reported resource cleanup issue for leaks,
and we came across a couple of possibly questionable things in the way
things are handled in CloseableHttpClient for the response handler
variant code below. Wanted to ask for comment. In execute():
try (final ClassicHttpResponse response = doExecute(target,
request, context)) {
try {
final T result = responseHandler.handleResponse(response);
final HttpEntity entity = response.getEntity();
EntityUtils.consume(entity);
return result;
} catch (final HttpException t) {
// Try to salvage the underlying connection in case of
a protocol exception
final HttpEntity entity = response.getEntity();
try {
EntityUtils.consume(entity);
} catch (final Exception t2) {
// Log this exception. The original exception is more
// important and will be thrown to the caller.
LOG.warn("Error consuming content after an
exception.", t2);
}
throw new ClientProtocolException(t);
}
}
First, if the response handler throws an unchecked RuntimeException or
a Throwable, the EntityUtils.consume(entity) will never get called. In
order to ensure absolutely that the entity is closed, would it not be
better to catch Throwable there instead of HttpException?
Second, in the implementation of EntityUtils.consume(entity):
public static void consume(final HttpEntity entity) throws
IOException {
if (entity == null) {
return;
}
if (entity.isStreaming()) {
Closer.close(entity.getContent());
}
}
is there some reason why it calls close() on the entity *content*,
rather than on the HttpEntity itself? The HttpEntity itself is also
Closeable, but it itself is never actually closed.
Examining all the currently existing HttpEntity impls, it does seem
that all of them that are of the streaming type implement their close()
by merely calling close() on their content... So practically speaking
the above code isn't "wrong" in terms of behavior of existing impls.
But it's also a bit counterintuitive when it would seem more natural to
just close() the HttpEntity, which will take care of close() on its
content if applicable. Also safer in the case of a new (or custom)
HttpEntity type which does more in its close() than merely close its
content stream.
Thanks for any insight you can provide. Happy to open a bug report if
that is more appropriate.
--Brent
