jdaugherty commented on code in PR #16415:
URL: https://github.com/apache/grails-core/pull/16415#discussion_r4118021165
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/transactions/TransactionObject.java:
##########
@@ -77,7 +118,7 @@ public boolean isNewSession() {
@Override
public boolean isRollbackOnly() {
Review Comment:
`flush()` just below still flushes `sessionHolder.getSession()`, the session
bound on top, while commit and rollback now act on the transaction's own
session. A `status.flush()` made while a session is pushed on top flushes the
wrong one. Using `transactionSession` when it is set, as
`transactionSession(txObject)` does in the manager, keeps the three consistent.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/transactions/DatastoreTransactionManager.java:
##########
@@ -224,17 +280,52 @@ protected void doSetRollbackOnly(DefaultTransactionStatus
status) throws Transac
TransactionObject txObject = (TransactionObject)
status.getTransaction();
status.setRollbackOnly();
txObject.getSessionHolder().setRollbackOnly();
+ // A joined transaction failed, so the whole transaction will roll
back. Discard what is queued
+ // now: flushed explicitly before then, it would be written, and
without a server-side
+ // transaction nothing could take it back.
+ Session session = txObject.getSessionHolder().getSession();
+ if (session != null) {
+ session.clear();
+ }
+ }
+
+ /**
+ * A committed or rolled back transaction can no longer be joined: code
run from its
+ * {@code afterCommit} or {@code afterCompletion} callbacks begins one of
its own, rather than joining
+ * a transaction that will not commit again and losing its writes.
+ */
+ private static void ended(TransactionObject txObject) {
+
txObject.getSessionHolder().setTransactionActive(txObject.getTransactionSession(),
false);
+ }
+
+ /**
+ * The session the transaction began on, which its commit flushes and its
rollback clears, even if
+ * another has been bound on top of it since; the holder's current session
for a transaction begun
+ * by a subclass that does not record one.
+ */
+ private static Session transactionSession(TransactionObject txObject) {
+ Session session = txObject.getTransactionSession();
+ return session != null ? session :
txObject.getSessionHolder().getSession();
}
@Override
protected void doCleanupAfterCompletion(Object transaction) {
TransactionObject txObject = (TransactionObject) transaction;
+ SessionHolder sessionHolder = txObject.getSessionHolder();
- // Un-bind the session holder from the thread.
- if (txObject.isNewSessionHolder()) {
-
DatastoreUtils.closeSession(txObject.getSessionHolder().getSession());
+ ended(txObject);
+ // A joined transaction that failed marked the holder rollback-only. A
pre-bound holder outlives
+ // this transaction, and the next one begun on it must not inherit
that mark.
+ sessionHolder.resetRollbackOnly();
Review Comment:
The rollback-only mark is holder-wide, but a holder carries a stack of
sessions, and a transaction begun on a session that `withNewSession` pushed on
top runs its own `doCleanupAfterCompletion` against the same holder. Two things
follow once a failed participant has marked the outer transaction rollback-only:
1. The new-session transaction cannot commit.
`DefaultTransactionStatus.isGlobalRollbackOnly()` reads
`TransactionObject.isRollbackOnly()`, which is the shared holder's mark.
Spring's `TransactionTemplate` throws `UnexpectedRollbackException` from that
transaction; GORM's `withTransaction`, whose `inheritRollbackOnly` copies the
mark onto the status, rolls it back silently. That is the "record the failure
in a session of its own" pattern the new docs describe as separate ("a
transaction begun in that session is its own, and commits when it returns").
2. This line then erases the outer transaction's mark, so the outer commit
goes ahead although a participant failed.
Run against this head on the simple map datastore
(`grails-datamapping-core-test`) and on a replica set with
`grails.mongodb.transactional` on:
```groovy
TestEntity.withTransaction {
entity('A').save()
try {
TestEntity.withTransaction {
entity('B').save()
throw new IllegalStateException('inner')
}
}
catch (IllegalStateException ignored) {
}
TestEntity.withNewSession {
TestEntity.withTransaction { entity('audit').save() }
}
entity('C').save()
}
```
No exception is thrown anywhere, `audit` is never written, and `C` is
committed: `TestEntity.list()*.name == ['C']` on both. Without the
`withNewSession` block the same code rolls back as a whole, as the TCK feature
checks. Through `TransactionTemplate` the inner `withNewSession` transaction
throws `UnexpectedRollbackException` and the outer still commits (a mock-level
check of the manager alone shows `transaction.commit()` invoked).
Both go away if the mark is kept per transaction session, the way
`transactionSessions` already is: `doSetRollbackOnly` marks
`getSessionHolder().getSession()` (the session `isExistingTransaction`
matched), `TransactionObject.isRollbackOnly()` asks for its transaction
session, falling back to the holder's current session for a participant, and
cleanup resets only `transactionSession(txObject)`. The holder-wide
`setRollbackOnly()`/`isRollbackOnly()` inherited from `ResourceHolderSupport`
can stay for anything that calls them directly. Hibernate is not affected only
because its `withNewSession` binds a separate holder. The scenario above would
make a good sixth TCK feature next to the existing `withNewSession` one.
##########
grails-data-mongodb/docs/src/docs/asciidoc/introduction/upgradeNotes.adoc:
##########
@@ -53,6 +53,23 @@ class UserProfile {
See <<idGeneration,Identity Generation>> for the full description of
`storedAs`.
+==== A Transaction Started Inside Another Joins It
+
+A transactional service method called from another, or `withTransaction`
inside `withTransaction`, used to begin a second transaction on the same
session: it committed on its own, and the surrounding transaction's commit then
did nothing, so the surrounding transaction's later writes could be lost. It
now joins the surrounding transaction, and everything commits once when that
one does, as on Hibernate.
+
+The effect most applications will notice: GORM for MongoDB does not flush
before a query while a transaction is in progress, so a write a joined call
leaves queued is not visible to a later query in the same transaction. It used
to be, because the inner transaction flushed it when it returned:
Review Comment:
Not for this PR, but worth an issue: `MongoQuery.flushBeforeQuery` skips the
flush whenever synchronization is active because, without a server-side
transaction, a flushed write cannot be taken back. With
`grails.mongodb.transactional` on, that reason no longer holds: a flush inside
the `ClientSession` transaction is aborted with it. Flushing before a query
when the session `hasActiveTransaction()` would give the Hibernate visibility
(a joined call's writes visible to later queries, `@Rollback` tests unchanged)
for exactly the configuration this PR makes work, and this note would then
apply only to the non-transactional setup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
