HTTPCLIENT-2432: fix connection-pool leak in ResponseEntityProxy.cleanup() when disconnectEndpoint() throws

[krocodl]

Jira: https://issues.apache.org/jira/browse/HTTPCLIENT-2432

Summary

ResponseEntityProxy#cleanup() invokes two sequential calls without a finally:

private void cleanup() throws IOException {
    if (this.execRuntime != null) {
        if (this.execRuntime.isEndpointConnected()) {
            this.execRuntime.disconnectEndpoint();   // (A)
        }
        this.execRuntime.discardEndpoint();          // (B)
    }
}

(A) InternalExecRuntime#disconnectEndpoint() calls endpoint.close() and can throw IOException when the underlying socket is in a broken state (e.g. RST mid-body from an unstable tunnel). It does not clear endpointRef.

(B) InternalExecRuntime#discardEndpoint() is the only path that returns the lease to the pool — it does endpointRef.getAndSet(null) and, inside a finally, manager.release(endpoint, null, TimeValue.ZERO_MILLISECONDS).

When (A) throws, (B) is skipped. endpointRef stays populated, manager.release(...) is never invoked, and PoolingHttpClientConnectionManager continues to count the connection as leased for the entire lifetime of the client. Under any retry stack above the client, a single flapping tunnel can accumulate many leaked slots in one run.

Fix

Wrap (A) in try and put (B) in finally so the lease is always returned:

private void cleanup() throws IOException {
    if (this.execRuntime != null) {
        try {
            if (this.execRuntime.isEndpointConnected()) {
                this.execRuntime.disconnectEndpoint();
            }
        } finally {
            this.execRuntime.discardEndpoint();
        }
    }
}

Safety:

• discardEndpoint() is idempotent — first line is endpointRef.getAndSet(null), so a redundant call after a prior releaseEndpoint() / discardEndpoint() is a no-op.
• Pool release inside discardEndpoint() is already in a finally, so endpoint.close(CloseMode.IMMEDIATE) throwing on a dead socket does not prevent manager.release(...).
• On the healthy path observable behavior is unchanged: endpointRef is already null when cleanup() runs (cleared by releaseConnection() / discardConnection()), so the added finally no-ops.

Test plan

• Added TestResponseEntityProxy#testCleanupDiscardsEndpointWhenDisconnectEndpointThrows — mocks ExecRuntime.disconnectEndpoint() to throw IOException, verifies discardEndpoint() is still invoked. Without the fix this fails (discardEndpoint() never called); with the fix it passes.
• mvn -pl httpclient5 -am test -Dtest=TestResponseEntityProxy — 4/4 green locally.

DCO

Commit is Signed-off-by: Victor Alekseev <krocodl@gmail.com>.

[ok2c]

@krocodl The release notes changes are misplaced. 5.7-alpha1 has already been released.

I will make sure you will get credit for the fix and a mention in the release notes. Please put everything you want to be in the release notes to the commit message. Looks good otherwise.

[krocodl]

@ok2c done

[arturobernalg]

LGTM