Is calling transaction supposed to set autocommit to false automatically?

[nidu]

Hello, i have a question about transactions.

Is wrapping code in transaction with query.transaction().in(() -> ...) supposed to automatically set autocommit to false? I noticed that my changes are not rolled back unless i do connection.setAutoCommit(false);. I'm creating connection with DriverManager.getConnection.

I had an expectation that wrapping code in a transaction turns off autocommit, but now i'm confused.

Thanks

[zsoltherpai]

I need a bit more context on how you instantiated Query and what you do inside the in(...) callback.

I have a possible explanation, but it might be a long shot:
Did you make an instance of Query based on a specific Connection object, then executed a query using the raw Connection object?

When transaction().in(...) is used, autocommit=false is not set yet. Transaction is deferred, it's only opened right before the first query is executed. So transaction().in(.. no queries executed..) will not even open a transaction. transaction().in(... executes a query at some point... ) will open one (autocommit=true) just in time. This is implemented so in order to minimize the number - and duration - of transactions. The catch is the library won't detect the query happening - and won't start a transaction - if you don't use the Query api, for instance run a query using the Connection object directly.

A solution for this would be easy to add to the lib, for instance an option to start the transaction eagerly: eg transaction().eager().in(... queries not using the Query api...)

If you use the Query api inside in(...) as your first query then it's an issue and we need to dig somewhat deeper.

[nidu]

That's how I get Query. Here i have one Connection instance for testing purposes.

TestDataSourceImpl(String url, String userName, String password) throws SQLException {
    this.connection = DriverManager.getConnection(url, userName, password);
    this.connection.setAutoCommit(false);
}

// ...
    
public FluentJdbc getFluentJdbc(String url, String userName, String password) {
    return new FluentJdbcBuilder()
            .afterQueryListener(exec -> {
                logger.fine(
                        String.format("Query took %s ms: %s", exec.executionTimeMs(), exec.sql())
                );
                exec.sqlException().ifPresent(e ->
                        logger.log(Level.SEVERE, String.format("Error running query %s", exec.sql()), e)
                );
            })
            .connectionProvider(query -> {
                query.receive(this.connection);
            })
            .build();
}

And I only work using Query API, I never touch the connection outside this connectionProvider.

I'm also tracing and trying to figure out why is it happening.

[nidu]

Ok, i've got it. It's mostly my mistake i think.

Here's my scenario. I'm testing my service and would like to rollback in the end of every test. For this i've made following method. The idea is that if handler throws anything - transaction is rolled back. If everything is ok - i'm throwing Rollback in the end anyway, so rollback is kinda guaranteed.

private void withRollback(Runnable action) {
    try {
        query.transaction().in(() -> {
            action.run();
            throw new Rollback();
        });
    } catch (Rollback ignored) { }
}

But this is not working.

action runs tests inside, which may throw junit AssertionError, which is not an Exception (Throwable -> Error -> AssertionError). This is not caught inside inNewTransaction. Method for reference with some comments:

private <T> T inNewTransaction(Supplier<T> operation, Map<ConnectionProvider, Connection> cons) {
    try {
        List<T> result = new ArrayList<>(1);
        queryInternal.connectionProvider.provide(
                con -> {
                    Boolean originalAutocommit = null;
                    try {
                        isolation(con);
                        originalAutocommit = con.getAutoCommit();
                        cons.put(queryInternal.connectionProvider, con);
                        try {
                            result.add(operation.get()); // AssertionError is thrown here
                        } catch(RuntimeException e) {
                            con.rollback(); // Is not executed
                            throw e;
                        }
                        con.commit(); // Is not executed
                    } catch(SQLException e) {
                        throw new FluentJdbcSqlException("Error executing transaction", e);
                    } finally {
                        restoreOriginalAutocommit(con, originalAutocommit); // Executed
                        removeTransactionedConnection(cons);
                    }
                }
        );
        return result.get(0);
    } catch(SQLException e) {
        // should not occur
        throw new FluentJdbcSqlException("Error executing transaction.", e);
    }
}

So neither rollback nor commit is called here. So depending on initial autoCommit value (set right after creating a connection, before calling any Fluent methods):

• true - Fluent sets autoCommit to false correctly before running queries. Later when Error is thrown - line restoreOriginalAutocommit(con, originalAutocommit); sets autoCommit back to true and also commits the data. That's why all my changes were saved inside my withRollback method.
• false - Fluent doesn't change autoCommit anywhere since it's already false. But when tests are finished - neither commit nor rollback is called and connection for some reason remains hanging. Probably connection.close() is badly implemented in the underlying driver. So my is not saved because of hanging transaction which dies in about an hour.

Combination of factors above made initial impression.

So about the fact that neither rollback nor commit is called if non RuntimeError is thrown from inside the query - do you think it should be handled?

UPD: here's my withRollback method to handle it:

private void withRollback(Runnable action) {
    try {
        query.transaction().in(() -> {
            try {
                action.run();
            } catch (Throwable e) {
                logger.log(Level.SEVERE, "Rollback inside", e);
                throw new RuntimeException("Error running action");
            }
            throw new Rollback();
        });
    } catch (Rollback ignored) { }
}

[zsoltherpai]

The updated variant looks good. You might want to drop the log line and just pass e as a second argument in the RuntimeException constructor to get a single log entry for an error.

The lib not catching Errors is intentional, since that might have nasty consequences. However, catching AssertionError specifically seems quite harmless. I can make that change if you want, then you won't need to work around that at least.

[nidu]

Adding special case for AssertionError is maybe to specialized since different test frameworks may have different types for it. What types of nasty things you think can happen? I just barely knew about Error type in Java and still trying to grasp it. I'd probably leave it as is then, but it would be really useful to have in documentation for transactions.

…

On Fri, Aug 9, 2019, 13:39 Zsolt Herpai ***@***.***> wrote: The updated variant looks good. You might want to drop the log line and just pass e as a second argument in the RuntimeException constructor to get a single log entry for an error. The lib not catching Errors is intentional, since that might have nasty consequences. However, catching AssertionError specifically seems quite harmless. I can make that change if you want, then you won't need to work around that at least. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#71?email_source=notifications&email_token=AADKBSNQ7NFTP3H4477NBRDQDUUMVA5CNFSM4IJ6CKJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD36AYAI#issuecomment-519834625>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADKBSM7E3K7KIP5ULR3RKDQDUUMVANCNFSM4IJ6CKJQ> .

[zimmi]

Just for reference, the Spring TransactionTemplate rolls back on Throwable, with some special handling for sneakily thrown undeclared checked exceptions. Since that has worked fine in practice, maybe it makes sense to align the implementation?