SpringBoot Cleanup all resources in RedisHealthIndicator

This block of code in the Spring Boot Actuator, RedisHealthIndicator should be restructured as:

try (RedisConnection connection = RedisConnectionUtils.getConnection(this.redisConnectionFactory)) {
    doHealthCheck(builder, connection);
}
finally {
    RedisConnectionUtils.releaseConnection(connection, this.redisConnectionFactory);
}

Alternatively, this could be structured as:

RedisConnection connection = null;
try {
    connection = RedisConnectionUtils.getConnection(this.redisConnectionFactory);
    doHealthCheck(builder, connection);
}
finally {
    RedisConnectionUtils.releaseConnection(connection, this.redisConnectionFactory);
}

If any kind of RuntimeException is thrown from the RedisConnectionFactory.getConnection() method (indirectly via the Spring Data Redis RedisConnectionUtils.getConnection(:RedisConnectionFactory) method, here then here), then there is a possibility that system resources may not get properly cleaned up (for instance, a Socket is still allocated, even connected, but fails during a TLS handshake, or perhaps during username/password auth).

A reference to a connection could be returned on line 49, but won't be released properly if the call throws an exception as it exists outside the try-finally block.

All RedisConnection(s) are AutoCloseable (Javadoc).

This was uncovered during the analysis of Spring Data Redis Issue #2619.

Comment From: jxblum

To clarify, I don't thing Spring Boot (Actuator, and the RedisHealthIndicator in particular) is causing the user's perceived TCP connection leak in this case, nor is Spring Data Redis. But, after reviewing the code in both Spring Data Redis and Spring Boot, I do think this is an opportunity for a small improvement in RedisHealthIndicator.

Comment From: wilkinsona

Thanks, John, but I'm not sure I understand the problem. If an exception is thrown from RedisConnectionUtils#getConnection there will be no RedisConnection available for us to close. I think the second option you described above shows this most clearly:

RedisConnection connection = null;
try {
    connection = RedisConnectionUtils.getConnection(this.redisConnectionFactory);
    doHealthCheck(builder, connection);
}
finally {
    RedisConnectionUtils.releaseConnection(connection, this.redisConnectionFactory);
}

If RedisConnectionUtils.getConnection(this.redisConnectionFactory) throws an exception, connection will remain null. The finally block with then call RedisConnectionUtils.releaseConnection(null, this.redisConnectionFactory);. That won't fail but it also won't do anything as releaseConnection returns immediately when conn is null:

public static void releaseConnection(@Nullable RedisConnection conn, RedisConnectionFactory factory) {
    if (conn == null) {
        return;
    }

Have I overlooked something? Can you please clarify if I have.

Comment From: jxblum

Hi Andy-

Sorry. On second thought, I think you are correct.

I have been bouncing back and forth between the user's problem (TCP connection leak) in the SD Redis ticket I referenced and what Spring Boot Actuator was doing inside the RedisHealthIndicator (in combination w/ SD Redis) that might possibly be causing this to happen. I think I am beginning to overthink it now.

When I saw the connection acquisition outside the try-finally block I thought, uh-oh! But, there is actually no way for the resource to be leaked by the framework in this case unless the driver itself is not properly releasing system resources, when some Exception is thrown.

This could be the case during a failed TLS handshake, or even simple username/password auth, since the TCP Socket would still be opened and established to do the handshake, or auth. Even in this case, a certain amount of responsibility still falls on users to manage the system resources properly for their context and UC/Reqs (such as, configuring SO_LINGER, SO_REUSEADDR, SO_TIMEOUT, etc).

For clarification (and quick sanity check), there is no repeated process (e.g. Thread) inside Boot to run any of the HealthIndicators in an automatic fashion is there? There isn't AFAIA, not unless the user coded up some automatic check themselves, or are maybe using a monitoring application/tool. Otherwise, it is all upon request by hitting the URL endpoint, correct?

So, outside of some syntactic sugar (and minimizing the scope of the connection variable), RedisHealthIndicator is probably fine as is.

NOTE: FYI, beyond opening and closing connections, RedisConnectionUtils additionally synchronizes with any configured transaction, which might add a bit of unnecessary overhead for such a simple health assessment (check), given, and I assume, there isn't a need for a transaction in this case.

Comment From: wilkinsona

Otherwise, it is all upon request by hitting the URL endpoint, correct?

Correct.