On Mon, 2023-08-28 at 16:48 -0400, Brent Putman wrote:
> 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);
> }
> }
>
This code is not about connection clean-up, this code is about
connection re-use. The resource clean-up is ensured by closing out the
response.
> 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?
>
One should almost never, never, ever catch Throwable.
> 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.
>
Again, this is about trying to salvage and re-use the connection, not
about resource clean-up. Besides, what usually all entities do in their
#close method is closing their content stream.
Oleg
> 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
