State inconsistency in LazyConnectionDataSourceProxy when connection settings fail #35980
Description
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; }