DefaultSqlSession doesn't ROLLBACK on close after SELECT

[ivanr]

I've been reading the code of DefaultSqlSession and I believe that transactions are not correctly closed when close() is called. The current behaviour is that rollback is submitted only for INSERT/UPDATE/DELETE statements, but not SELECT. Because there is no rollback, the same transaction continues and multiple logical operations may be called in the same transaction, with unpredictable side-effects. For example, a SELECT... FOR UPDATE could create a lock that is not released. Advisory locks wouldn't be released.

The correct behaviour is to ROLLBACK is any one query, including SELECTs.

EDIT The same problem exists with calling close() and rollback(). In our case, we always force commits and rollbacks, but there's no such option for the close operation.

MyBatis version

3.5.7

Database vendor and version

Possibly all, but I have Postgres in mind.

Test case or example project

None.

Steps to reproduce

With connection pooling configured, create a session object, submit some SELECT queries, then close the session without committing or rolling back.

Expected result

Closing SQL session will ROLLBACK the connection.

Actual result

There is no ROLLBACK. EDIT 2022-12-03 Transaction remains active in idle state until the connection is reused. Transaction is committed.

[harawata]

Hello @ivanr ,

I could be wrong, but this seems to be an issue that should be taken care of by the connection pool.
Did you verify this with an actual connection pool implementation?
If yes, which one?

[kazuki43zoo]

should be taken care of by the connection pool.

indeed!!

[kazuki43zoo]

The connection pool provided by MyBatis rollback real connection at before returning it into pool.

Other major OSS connection pool implementations rollback a real connection at before returning it by default behavior.

• HikariCP

• Tomcat DBCP

• Commons DBCP2

[ivanr]

@harawata I have so far only looked at the logic in DefaultSqlSession. I will do another check, this time with a connection pool. It may take me some time to respond because of the upcoming holidays, but I will get back to you. Thank you.

[harawata]

Reading your comments on #2156 (comment) , I think your issue may also be resolved by #2741 .

SELECT... FOR UPDATE is not the exact target of the change and, as we explained, the transaction will be closed by the connection pool if you used one.
Still, there might be cases where the new affectData comes in handy.

Please let me know if you think it's a different issue.

[ivanr]

@harawata Thank you. With affectData it will be possible to write mappers that behave correctly.

Prompted by this comment I went back to test the exact behaviour. I observed that, when a session is being closed, a commit is executed if the session is not dirty. Tracing through the code, I think this is the code in CachingExecutor that calls commit:

  @Override
  public void close(boolean forceRollback) {
    try {
      //issues #499, #524 and #573
      if (forceRollback) {
        tcm.rollback();
      } else {
        tcm.commit();
      }
    } finally {
      delegate.close(forceRollback);
    }
  }

Regarding MyBatis's tracking of "dirty" transactions and having to explicitly force commits, etc, what's the rationale behind it? Is it documented anywhere? I think it would be easier for everyone if commit and rollback were just that, and if a rollback were used before a connection is returned to a pool.

Given the above code, where either commit or rollback are executed, why is commit preferred despite the potential unwanted side-effects? Are there any databases where commits are much faster?

NOTE I edited this comment after realising the the commit is seemingly not run by the connection pool.

[harawata]

Hello @ivanr ,

MyBatis does not explicitly call commit() on close.

If the transaction is committed on close, you probably are using "JDBC" transaction manager.
Here is an excerpt from the doc:

By default, it enables auto-commit when closing the connection for compatibility with some drivers.

And some drivers (including pgjdbc) internally commit the transaction when setAutoCommit(true) is called.
This may explain the behavior you observe.

Please see the doc for how to change the default behavior.
We couldn't change the default value to keep backward compatibility, but the non-default setting might work better for most DBs, I suspect.

---

I don't know why commit() and rollback() require force option when the session is not 'dirty' (because "it's not necessary", maybe?), but it's documented.

By default MyBatis does not actually commit unless it detects that the database has been changed by a call to insert, update or delete. If you've somehow made changes without calling these methods, then you can pass true into the commit and rollback methods to guarantee that they will be committed (note, you still can't force a session in auto-commit mode, or one that is using an external transaction manager). Most of the time you won't have to call rollback(), as MyBatis will do that for you if you don't call commit. However, if you need more fine-grained control over a session where multiple commits and rollbacks are possible, you have the rollback option there to make that possible.

(In #2741, I added "select with affectData enabled" to the list of actions)

p.s.
The tcm in CachingExecutor is TransactionalCacheManager which manages MyBatis' cache transaction, not the database transaction.
The 'dirty' flag is used to track cache status as well.