State inconsistency in LazyConnectionDataSourceProxy when connection settings fail

[NYgomets]

Description:

I discovered a state inconsistency issue in LazyConnectionDataSourceProxy where connection settings (such as transaction isolation level or auto-commit) are not reapplied after an initial failure. This leads to the reuse of "partially initialized" connections with incorrect configurations during retries.

Root Cause Analysis:

In LazyConnectionInvocationHandler.getTargetConnection(), the sequence of "connection acquisition" and "configuration application" lacks atomicity.

    if (this.target == null) {
        // Step 1: Assign connection immediately
        this.target = dataSource.getConnection();
        
        // Step 2: Apply settings (Failure point)
        if (this.transactionIsolation != null && ...) {
            // If this throws SQLException, 'this.target' remains assigned but dirty.
            this.target.setTransactionIsolation(this.transactionIsolation); 
        }
    }
    else {
        // On retry: logic enters here because 'this.target' is not null.
        // The configuration logic is skipped, and the dirty connection is returned.
        return this.target;
    }
    return this.target;
}

Scenario:

Client requests TRANSACTION_SERIALIZABLE isolation level.

Proxy stores this in memory.

Client triggers connection fetch (e.g., createStatement()).

Proxy fetches physical connection: this.target = dataSource.getConnection() (Success).

Proxy attempts to apply settings: this.target.setTransactionIsolation(...) (Fails with SQLException).

Exception propagates to the client.

Client retries (e.g., Spring Retry, Loop): clientConn.createStatement().

Proxy checks if (this.target == null) -> False (Already assigned).

Result: The initialization logic is skipped, and the dirty connection is returned. The query executes with the default isolation (e.g., READ_COMMITTED) instead of SERIALIZABLE.

Impact:

Data Consistency Risk: Queries execute with incorrect isolation levels or auto-commit modes, potentially leading to race conditions, phantom reads, or unintended commits.

Silent Failure: No exception is thrown on retry, making this bug difficult to detect in production.

Reproduction:

Here is a JUnit 5 + Mockito test case demonstrating the issue.

@Test
    @DisplayName("Bug reproduction: Connection reused without reapplying settings after initialization failure")
    void verifyStateCorruptionOnInitializationFailure() throws SQLException {
        // 1. Mock setup
        DataSource targetDataSource = mock(DataSource.class);
        Connection physicalConnection = mock(Connection.class);

        // Physical connection acquisition always succeeds
        given(targetDataSource.getConnection()).willReturn(physicalConnection);
        
        // Mock default values (triggers internal checkDefaultConnectionProperties)
        given(physicalConnection.getAutoCommit()).willReturn(true);
        given(physicalConnection.getTransactionIsolation())
              .willReturn(Connection.TRANSACTION_READ_COMMITTED);
        
        // Mock createStatement to return a dummy object (to pass null-checks later)
        given(physicalConnection.createStatement()).willReturn(mock(Statement.class));

        // [Key] setTransactionIsolation always throws exception
        int requiredIsolation = Connection.TRANSACTION_SERIALIZABLE;
        doThrow(new SQLException("Simulated Network Error: Isolation Set Failed"))
              .when(physicalConnection).setTransactionIsolation(requiredIsolation);

        LazyConnectionDataSourceProxy proxyDataSource = new LazyConnectionDataSourceProxy(targetDataSource);

        // Step 1: First attempt - fails
        Connection clientConn = proxyDataSource.getConnection();
        clientConn.setTransactionIsolation(requiredIsolation);

        assertThatThrownBy(() -> clientConn.createStatement())
              .isInstanceOf(SQLException.class)
              .hasMessageContaining("Simulated Network Error");

        // Step 2: Retry - succeeds silently without reapplying settings (BUG)
        Statement stmt = clientConn.createStatement();

        // Step 3: Verification
        
        // Physical connection fetched twice (1. checkDefaultConnectionProperties + 2. first attempt)
        // If fixed, it should be fetched again or re-initialized on retry.
        verify(targetDataSource, times(2)).getConnection();

        // [Key verification] setTransactionIsolation called only once (during the failed attempt).
        // It was NOT retried in Step 2.
        verify(physicalConnection, times(1)).setTransactionIsolation(requiredIsolation);

        // Statement created successfully with WRONG isolation level
        assertThat(stmt).isNotNull();
    }

Suggested Fix:

We should ensure atomicity during connection initialization. A robust approach would be to use a local variable for initialization and assign it to this.target only after all settings are successfully applied.

private Connection getTargetConnection(Method operation) throws SQLException {
    if (this.target == null) {
        // Use a local variable to ensure atomicity
        Connection target = dataSource.getConnection();
        
        try {
             // Apply settings to the local variable
             if (this.transactionIsolation != null && ...) {
                 target.setTransactionIsolation(this.transactionIsolation);
             }
             if (this.autoCommit != null && ...) {
                 target.setAutoCommit(this.autoCommit);
             }
             
             // ... apply other settings ...

             // Assign to member variable only after successful initialization
             this.target = target;
        }
        catch (SQLException ex) {
            // Cleanup the physical connection immediately
            try {
                target.close();
            }
            catch (SQLException closeEx) {
                // Log if necessary
            }
            throw ex;
        }
    }
    return this.target;
}