Skip to content

LettuceConnectionFactory.destroy(…) releases cluster connections after shutting down the connection pool #2330

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
fightchwang opened this issue May 24, 2022 · 2 comments
Assignees
Labels
type: bug A general bug

Comments

@fightchwang
Copy link

fightchwang commented May 24, 2022

public RedisClusterCommands<byte[], byte[]> getResourceForSpecificNode(RedisClusterNode node) {
Assert.notNull(node, "Node must not be null!");
if (connection == null) {
synchronized (this) {
if (connection == null) {
this.connection = connectionProvider.getConnection(StatefulRedisClusterConnection.class);
}
}
}
return connection.getConnection(node.getHost(), node.getPort()).sync();
}
@Override
@SuppressWarnings("unchecked")
public void returnResourceForSpecificNode(RedisClusterNode node, Object resource) {}
@Override
public void destroy() throws Exception {
if (connection != null) {
connectionProvider.release(connection);
}
}
}

if i execute RedisConnectionCommands::ping command via a redistemplate, then should this connection variable to be reset to null in the callback method returnResourceForSpecificNode ?

if not, then when the destroy callback was called druing spring exit , the connectionProvider was already closed, calling connectionProvider.release(connection); would result in exception :

Returned connection io.lettuce.core.cluster.StatefulRedisClusterConnectionImpl@2d3456b was either previously returned or does not belong to this connection provider, did i understand right?

public void release(StatefulConnection<?, ?> connection) {
GenericObjectPool<StatefulConnection<?, ?>> pool = poolRef.remove(connection);
if (pool == null) {
AsyncPool<StatefulConnection<?, ?>> asyncPool = asyncPoolRef.remove(connection);
if (asyncPool == null) {
throw new PoolException("Returned connection " + connection
+ " was either previously returned or does not belong to this connection provider");
}
discardIfNecessary(connection);
asyncPool.release(connection).join();
return;
}
discardIfNecessary(connection);
pool.returnObject(connection);
}

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 24, 2022
@mp911de mp911de added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels May 24, 2022
@mp911de mp911de self-assigned this May 24, 2022
@mp911de
Copy link
Member

mp911de commented May 24, 2022

if i execute RedisConnectionCommands::ping command via a redistemplate, then should this connection variable to be reset to null in the callback method returnResourceForSpecificNode ?

LettuceClusterNodeResourceProvider uses a Lettuce cluster connection to obtain an individual connection to a cluster node to run a particular command. Lettuce internally manages the node connection lifecycle. In LettuceClusterNodeResourceProvider it matters to keep a cluster connection until the LettuceClusterConnection gets closed.

The problem that you see is an ordering issue without real impact, it's mostly logging an exception.

Shutting down LettuceConnectionFactory first closes all connection providers, cleaning out the connection pools. Once the pools are closed, the clusterCommandExecutor holding a connection attempts to return (close) its connection. That disposal leads to the PoolException along with logging.

We need to fix the issue.

@mp911de mp911de changed the title LettuceClusterConnection.LettuceClusterNodeResourceProvider releases an already released connection? LettuceConnectionFactory.destroy(…) releases cluster connections after shutting down the connection pool May 24, 2022
mp911de added a commit that referenced this issue May 24, 2022
We now close the cluster command executor before cleaning up the connection pools so that we first release all held connections before pruning the connection pools.
Previously, the pools were pruned first leading to an attempt to return the connection held by the cluster command executor causing a PoolException.

Closes #2330
mp911de added a commit that referenced this issue May 24, 2022
We now close the cluster command executor before cleaning up the connection pools so that we first release all held connections before pruning the connection pools.
Previously, the pools were pruned first leading to an attempt to return the connection held by the cluster command executor causing a PoolException.

Closes #2330
@fightchwang
Copy link
Author

@mp911de thx for your reply, as you said, it's an ordering issue without real impact, it's mostly logging an exception.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

3 participants