Preserve Connection readOnly state for DataSource with defaultReadOnly configuration

[NYgomets]

Current Behavior

DataSourceTransactionManager does not restore the original readOnly state of database connections after transaction completion. While the transaction isolation level is properly backed up and restored, the readOnly flag is unconditionally reset to false.

Code Analysis

In doBegin() (line ~325):

Integer previousIsolationLevel = DataSourceUtils.prepareConnectionForTransaction(con, definition);
txObject.setPreviousIsolationLevel(previousIsolationLevel); // ✓ Backed up

txObject.setReadOnly(definition.isReadOnly()); // ✗ Current transaction's flag, not the original connection state

In doCleanupAfterCompletion() (line ~425):

DataSourceUtils.resetConnectionAfterTransaction(
    con, txObject.getPreviousIsolationLevel(), txObject.isReadOnly());
    //                                          ^^^ Passes current transaction's readOnly flag

In DataSourceUtils.resetConnectionAfterTransaction() (line ~287):

if (resetReadOnly) {
    con.setReadOnly(false); // ✗ Unconditionally resets to false
}

Expected Behavior

The connection's original readOnly state should be preserved and restored after transaction completion, similar to how the isolation level is handled.

Impact

1. Driver optimization loss: Some JDBC drivers (e.g., PostgreSQL) apply optimizations when readOnly=true. Unconditionally resetting to false loses these optimizations for subsequent transactions.

2. Unnecessary database queries: Drivers like MySQL execute SET SESSION TRANSACTION READ ONLY/READ WRITE commands. Incorrect state restoration causes unnecessary round-trips.

3. Connection pool state inconsistency: If a connection pool configures connections with specific default states, this behavior can leave the pool in an inconsistent state.

Steps to Reproduce

// Assume a connection pool that sets defaultReadOnly=true
BasicDataSource dataSource = new BasicDataSource();
dataSource.setDefaultReadOnly(true); // All connections start as readOnly=true

DataSourceTransactionManager tm = new DataSourceTransactionManager(dataSource);

@Transactional(readOnly = true)
public void readOnlyOperation() {
    // Connection comes in as readOnly=true
    // Transaction executes
    // After transaction: connection is reset to readOnly=false ✗
    // Connection returned to pool with wrong state
}

Environment

• Spring Framework: 6.x (main branch)
• Affected classes:
  • org.springframework.jdbc.datasource.DataSourceTransactionManager
  • org.springframework.jdbc.datasource.DataSourceUtils

Proposed Solution

Add previousReadOnly field to DataSourceTransactionObject and restore the original state after transaction completion, following the same pattern as previousIsolationLevel.

[jhoeller]

Dealing with the JDBC read-only flag is unfortunately rather involved. Some JDBC drivers do not support it all, some implement Connection#isReadOnly() as a rather expensive determination of the target database setup (for example with H2) rather than just returning the local Connection#setReadOnly-set flag. For a recent refinement to optimize our common interaction, see #23747.

In common practice, the read-only flag is meant as a hint rather than an actual property of the database interaction: neither is setReadOnly(true) enforcing read-only operations only, nor is setReadOnly(false) strictly necessary for actual read-write operations. FWIW most of the Java ecosystem therefore does not interact with Connection#setReadOnly at all, even if some drivers have good behavior implemented there. Spring does try to integrate that optional driver behavior - which is rather unique to Spring.

Most importantly, note that DataSourceTransactionManager does not call setReadOnly(false) for beginning standard transactions (without a read-only flag in the transaction definition). This means that it fundamentally assumes the read-only flag to be false by default. DataSourceTransactionManager is simply not designed for interactions with read-only databases or connection pools with a default read-only status - which is why it always calls setReadOnly(false) before returning the connection to the pool.

In that sense, the current behavior is as designed. Apache DBCP's defaultReadOnly flag is not expected to be used with Spring - and other connection pools do not offer such a default flag anyway, as far as I'm aware.

An important note if you are concerned about unnecessary database interactions: Spring's LazyConnectionDataSourceProxy is highly recommendable in-between DataSourceTransactionManager and the target connection pool there, avoiding the entire connection retrieval for "empty" interactions (where no actual statements get executed due to caching or the like), and also offering dedicated support for read-only DataSource configuration (see LazyConnectionDataSourceProxy#setReadOnlyDataSource).

All of that said, I can see scenarios where DataSourceTransactionManager could run against H2 or HSQLDB in a database-level read-only setup, which in the case of HSQLDB produces errors on setReadOnly(false) calls (which we suppress on resetting a transactional connection - but that's obviously not ideal). Also, a fork of Apache DBCP is present in Tomcat, offering a pool-level defaultReadOnly flag as well. Overall, this makes me reconsider whether we could leniently support such default read-only setups.

As a consequence, I've prepared a refinement where DataSourceTransactionManager lazily determines the default read-only status on the first obtained connection - assuming that this reflects a pool-level default, therefore avoiding a check for every transaction. This provides behavior similar to LazyConnectionDataSourceProxy#setReadOnlyDataSource: when preparing a transactional connection (skipping the setReadOnly(true) call) as well as when resetting it (skipping the setReadOnly(false) attempt then).

Note that this is not following the pattern of previousIsolationLevel but rather relies on the optional hint nature of setReadOnly, simply not calling that method at all for a default read-only setup, in alignment with existing behavior that we provide for LazyConnectionDataSourceProxy#setReadOnlyDataSource.

[github-actions]

Fixed via a330277

[NYgomets]

Thank you so much for the detailed and insightful response

It helped me learn a great deal about the complex behaviors of JDBC drivers that I had previously overlooked, such as the 'expensive' cost of isReadOnly() calls in H2.

In fact, I had attempted to fix this in my local environment by following the previousIsolationLevel pattern. When I ran the tests, I confirmed that many existing tests failed. This led me to suspect it was a 'more involved' problem that went beyond a simple code fix and might break other assumptions, though I was only guessing.

However, your explanation helped me understand the much bigger picture I was missing. I now see that this issue goes beyond just 'fixing broken tests' and involves the core fact that the behavior was 'as designed' (fundamentAally assuming false by default), as well as the performance risks of the isReadOnly() call itself.

Thank you also for preparing a refinement yourself that 'lazily determines' the default state on the first connection. It seems like a much safer and more efficient approach.

Thank you once again for broadening my perspective and for this learning opportunity.

And, if you'd be open to it, I was wondering if I could try to take on the refinement you mentioned—the one that 'lazily determines' the default read-only state on the first connection?

If you've already prepared the patch yourself, I completely understand. But if you are looking for a contribution, I would be happy to work on it.

[jhoeller]

@NYgomets thanks for raising this in the first place and for your deep analysis, just as with your recent pull requests!

As for the approach I've suggested above, I've rolled it in a couple of hours ago. It's ready for testing in the latest 6.2.13 snapshot :-)

[NYgomets]

That's fantastic news! Thank you so much

I'm really impressed with how quickly you prepared the refinement and rolled it in. It was a valuable learning experience for me to see how you balanced the 'as designed' behavior with the performance risks of the H2 driver, ultimately landing on the 'lazily determines' approach.

I'm glad my analysis could be helpful. I'll check it out in the 6.2.13 snapshot. Thanks again!