spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
56.6k stars 38.13k forks source link

Document LazyConnectionDataSourceProxy setup for routing datasource to act on transaction definition read-only flag #21415

Closed spring-projects-issues closed 11 months ago

spring-projects-issues commented 6 years ago

Kaan Ozdokmeci opened SPR-16876 and commented

In org.springframework.transaction.support.AbstractPlatformTransactionManager#getTransactionmethod,

the synchronization point is created after the transaction is created and a connection is attached to it via the doBegin method.

Having the synchronization point created afterwards prevents the ability to utilize an AbstractRoutingDatasource that is able to pick between read-only and read-write datasources for the transaction by inspecting 

org.springframework.transaction.support.TransactionSynchronizationManager#isCurrentTransactionReadOnly

when determining the appropriate datasource.

 

Is there any reason not to move synchronization point creation before the doBegin method so that routing datasources can act on the transaction definition?

If not happy to send a PR.


No further details from SPR-16876

OrangeDog commented 3 years ago

@jhoeller as the old assignee, can this be triaged?

kanakharaharsh commented 1 year ago

I faced the same issue recently. I had a quick workaround until this issue not getting fixed.

@Component
@Slf4j
public class CustomTransactionManager extends DataSourceTransactionManager {

    private static final long serialVersionUID = 1L;

    public CustomTransactionManager(DataSource dataSource) {
        super(dataSource);
    }

    @Override
    protected void doBegin(Object transaction, TransactionDefinition definition) {
        if (definition.getName().contains("package_substring")) {
            log.debug(String.format("Init. Transaction for MyApp Service: %s : %s", definition.getName(),
                    definition));
            TransactionSynchronizationManager.setCurrentTransactionIsolationLevel(
                    definition.getIsolationLevel() != TransactionDefinition.ISOLATION_DEFAULT
                    ? definition.getIsolationLevel()
                            : null);
            TransactionSynchronizationManager.setCurrentTransactionReadOnly(definition.isReadOnly());
            TransactionSynchronizationManager.setCurrentTransactionName(definition.getName());
        }
        super.doBegin(transaction, definition);
    }
}

As we can see, I am extending DataSourceTransactionManager and updating TransactionSynchronizationManager with the new TransactionDefinition before acquiring a connection for the JDBC transaction. This way we will always get the updated value in AbstractRoutingDataSource.determineCurrentLookupKey()

jhoeller commented 11 months ago

This is a close sibling to #19688, just with the initial case being the read-only flag. We mean to deal with these in combination for a JDBC theme in the 6.1.2 release.

As outlined on #19688, I am inclined to address the isolation level scenario here through an extension of SmartDataSource with a dedicated getConnection(int isolationLevel, boolean readOnly) method that DataSourceTransactionManager can call if the target DataSource implements it. IsolationLevelDataSourceRouter and a similar routing DataSource based on @Transactional(readOnly=...) can then take this into account directly when specified. This would allow us to preserve the semantics of our existing transaction synchronization arrangement which we cannot easily bend towards earlier exposure.

jhoeller commented 11 months ago

Experimenting with a few scenarios here, such an extension to SmartDataSource is rather involved in more complex setups, e.g. behind a JPA provider. In comparison to that, LazyConnectionDataSourceProxy is actually the simplest solution out: configuring both your transaction manager and your data access setup (JdbcTemplate or JPA setup) with a LazyConnectionDataSourceProxy conveniently allows for routing datasources to pick up a late-bound current isolation level within the transaction. And this comes with the extra benefit that connection contention is minimized and even avoided completely (with no connection ever fetched) for "empty" transactions which is quite common with JPA queries that can be answered from a cache.

As a consequence, I am going to turn this ticket into a documentation ticket for LazyConnectionDataSourceProxy. Maybe we'll even introduce a dedicated DataSourceRouter class for read-only vs read-write connections in the 6.1.2 timeframe since this is apparently as common a scenario as routing by isolation level.

krebsba4 commented 8 months ago

Doesn't seem to be working as expected in Spring 6.1.4

Using an AbstractRoutingDatasource wrapped with LazyConnectionDataSourceProxy (and setting the read-only datasource) during the initial DataSourceTransactionManager.doGetTransaction the DataSourceTransactionObject object is not marked as read-only, only the TransactionDefinition is. When this delegates to the AbstractRoutingDatasource it returns the read-write DataSource as the current TransactionSynchronizationManager is still not marked as read-only. It will still make a connection to the read-write DataSource and THEN set the read-only flag in the TransactionSynchronizationManager.

To make things more confusing, it looks like after this the LazyConnectionDataSourceProxy is doing its routing and returning the correct DataSource.

I was under the assumption that I wouldn't see any connection made to the read-write as the transaction should have been marked in the TransactionSynchronizationManager as read-only on transaction start, but that looks to not be the case. Only when using the work around mentioned by kanakharaharsh above does the transaction NOT start a connection to the read-write and only operates on the read-only.

jhoeller commented 8 months ago

The DataSourceTransactionManager interaction will still grab a Connection handle as usual, but with LazyConnectionDataSourceProxy inbetween, it should grab a lazy Connection from there, call a few methods to prepare that lazy Connection handle but not actually trigger a target Connection before an actual statement is being executed.

Also, as of 6.1.2, LazyConnectionDataSourceProxy has a setReadOnlyDataSource itself. There should not be a need for a separate routing DataSource for that standard use case anymore.

krebsba4 commented 8 months ago

Ok, after removing my AbstractRoutingDatasource I can see the connection is wrapped with the LazyConnectionProxy but see in my debug logs that it looks like my writer is starting and grabbing a connection from the pool. I don't think it's doing any action on that connection but will try to verify.

com.zaxxer.hikari.HikariDataSource       : writerDatasourcePool - Starting...
com.zaxxer.hikari.pool.HikariPool        : writerDatasourcePool - Added connection org.postgresql.jdbc.PgConnection@4c484e88
com.zaxxer.hikari.HikariDataSource       : writerDatasourcePool - Start completed.
o.s.j.d.DataSourceTransactionManager     : Acquired Connection [Lazy Connection proxy for target DataSource [HikariDataSource (writerDatasourcePool)]] for JDBC transaction
o.s.jdbc.datasource.DataSourceUtils      : Setting JDBC Connection [Lazy Connection proxy for target DataSource [HikariDataSource (writerDatasourcePool)]] read-only
o.s.j.d.DataSourceTransactionManager     : Switching JDBC Connection [Lazy Connection proxy for target DataSource [HikariDataSource (writerDatasourcePool)]] to manual commit
OrangeDog commented 7 months ago

That's because those lines are logged before a connection is actually made, so the toString() just delegates to the default DataSource. Unfortunately, Hikari doesn't have any logging to tell you when a connection is borrowed, nor from which pool.

I can confirm, via use of the debugger, that HikariDataSource.getConnection() is only called on the read-only DataSource.