Skip to content

Use before assignment of asyncDedicatedConnection in LettuceConnection #2984

Description

@oldium

I found issue while reading how Lettuce integration works in this method:

protected StatefulConnection<byte[], byte[]> doGetAsyncDedicatedConnection() {
StatefulConnection<byte[], byte[]> connection = getConnectionProvider().getConnection(StatefulConnection.class);
if (customizedDatabaseIndex()) {
potentiallySelectDatabase(this.dbIndex);
}
return connection;
}

The method creates a new connection, which is stored by the caller into a asyncDedicatedConnection attribute:

private StatefulConnection<byte[], byte[]> getOrCreateDedicatedConnection() {
if (this.asyncDedicatedConnection == null) {
this.asyncDedicatedConnection = doGetAsyncDedicatedConnection();
}
return this.asyncDedicatedConnection;
}

But it looks like the asyncDedicatedConnection is used before assignment in potentiallySelectDatabase() called by doGetAsyncDedicatedConnection():

private void potentiallySelectDatabase(int dbIndex) {
if (asyncDedicatedConnection instanceof StatefulRedisConnection<byte[], byte[]> statefulConnection) {
statefulConnection.sync().select(dbIndex);
}
}

Activity

  1. mp911de commented on Sep 5, 2024

    @mp911de
    Member

    This is a bug, good catch. We need to apply the database selection clearly on the newly created connection. Do you want to submit a pull request?

  2. oldium commented on Sep 5, 2024

    @oldium
    Author

    Please do it if you have time :-)

  3. added a commit that references this issue on Sep 8, 2024
    5457f77
  4. oldium commented on Sep 8, 2024

    @oldium
    Author

    Turns out the logic is broken here. select() refuses to use shared connection, so it executes the select command on a dedicated connection. This means that the potentiallySelectDatabase() call is superfluous in doGetAsyncDedicatedConnection, because the database change will be executed in select call anyway.

    I am not able to tell the difference between connection.sync().select() (used in potentiallySelectDatabase method) and invokeStatus().just(..., CommandType.SELECT, ...) (used in select method), though.

  5. added a commit that references this issue on Sep 8, 2024
    e47545a
  6. oldium commented on Sep 8, 2024

    @oldium
    Author

    Please check oldium@e47545a for possible fix. As I said, I am not sure whether the invokeStatus() (it uses RedisClusterAsyncCommands::dispatch, which is actually BaseRedisAsyncCommands::dispatch) is comparable/equivalent to connection.sync().select() call.

  7. self-assigned this
    on Sep 11, 2024
  8. added a commit that references this issue on Sep 11, 2024
    cf30543
  9. added 2 commits that reference this issue on Sep 12, 2024
    fd23412
    586d974
  10. oldium commented on Sep 12, 2024

    @oldium
    Author

    @christophstrobl I do not think the fix is correct and the change to the test proves it. The SELECT Redis command is executed twice, first time in RedisCommands.select() call and second time in the BaseRedisAsyncCommands.dispatch(SELECT) call. Please see my former comments.

  11. christophstrobl commented on Sep 13, 2024

    @christophstrobl
    Member

    thanks for the heads up - true - working on it.

  12. mp911de commented on Sep 13, 2024

    @mp911de
    Member

    You're right, we're setting now the database index twice. We had initially a different understanding. By removing the database index customization check and db selection in doGetAsyncDedicatedConnection(…) we can address the issue.

  13. added a commit that references this issue on Sep 15, 2024
    8777cf7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions