ScanIteration (JedisCommandIterationBase) connection leak with JedisSentineled

[veselov]

Not sure I fully understand what's going on, so looking for some idea on how to even approach fixing this.

            var jedis = new JedisSentineled(....);
            var it = j.scanIteration(100, cacheObjKey()+"*");
            // doesn't really matter what happens past the line above
            while (!it.isIterationCompleted()) {
                var data = it.nextBatchList().toArray(new String[0]);
                if (data.length > 0) {
                    j.del(data);
                }
            }

This leaks a connection for me.
JedisCommandIterationBase's constructor calls connectionProvider.getConnectionMap(), which is default ConnectionProvider's getConnectionMap(), which just allocates a connection, puts in a single-element map, and returns that. JedisCommandIterationBase is not a Closeable, and never clears that connection out (because it doesn't think it needs to).

It looks like pool-based connection providers must return a pool from a getConnectionMap(), like PooledConnectionProvider does. SentinelConnectionProvider doesn't do that. But it's also not easy for it to do that because its pool is volatile.

Any ideas how to work around this in the meantime?

[ggivo]

Hi @veselov

Thanks for reporting this. Good catch! I've verified the issue and can confirm the connection leak.

You're absolutely on the correct path by considering overriding getConnectionMap(). I believe something along these lines should resolve the connection leak:

@Override
public Map<?, Pool<Connection>> getConnectionMap() {
  return Collections.singletonMap(currentMaster, pool);
}

@Override
public Map<?, Pool<Connection>> getPrimaryNodesConnectionMap() {
  return Collections.singletonMap(currentMaster, pool);
}

Regarding the volatile pool consideration:

While the pool field is indeed volatile and can change during sentinel failover, this shouldn't be a problem for the iteration use case:

• Snapshot semantics: When scanIteration() is created, it captures a reference to the pool at that moment in time via getConnectionMap(). Even if a failover occurs and the pool is reassigned to a new instance, the iteration continues using its captured reference to the old pool.
• The edge case would be if a failover happens during iteration and the old master becomes completely unavailable, but that would fail regardless of the implementation approach.

I'll prepare a PR with this fix shortly.