codeconsole opened a new pull request, #16415: URL: https://github.com/apache/grails-core/pull/16415
## Problem A transaction started while another is in progress does not join it. `DatastoreTransactionManager` never overrides Spring's `isExistingTransaction`, whose default is `false`, so every nested `PROPAGATION_REQUIRED` call is treated as a new transaction: a `@Transactional` service called from another, `withTransaction` inside `withTransaction`, a `@ReadOnly` method called from a read-write one. The new transaction is begun on the same thread-bound session. Its commit completes the session's transaction, and the outer transaction's commit then finds that spent transaction and does nothing. #14391 reported this in 2020. What it does, measured with the TCK spec this PR adds, run against an unmodified `8.0.x`: | | MongoDB | simple map | Neo4j | |---|---|---|---| | outer saves A, inner `withTransaction` saves B, outer saves C | **A, B** (C never written) | A, B, C | A, B, C | | a caught inner failure, outer carries on and saves C | nothing committed | **C** committed | **C** committed | | `REQUIRES_NEW` inner commits, outer then throws | **inner, outer** | **inner, outer** | **inner, outer** | Nothing reports any of it. On MongoDB with a session bound for the request (open session in view), the lost writes are instead flushed when the request ends, outside any transaction, unless a read-only call left the session in `COMMIT` flush mode, in which case they are never written. With `grails.mongodb.transactional` (#15744) it is worse. Beginning the inner transaction calls `closeClientSessionQuietly()`, which aborts the outer transaction on the server, so the writes the outer one flushed *before* the call are discarded too, and its writes after the call go out session-less. `MongoTransactionSpec`'s `REQUIRES_NEW` feature passed for exactly this reason: the outer write was aborted by the inner begin, not isolated by suspend/resume. And a read-only transaction starts a server-side transaction, which pins its reads to the primary (with any other read preference every `@ReadOnly` read fails: `Read preference in a transaction must be primary`) and puts them under `transactionLifetimeLimitSeconds`. ## Change `DatastoreTransactionManager`, following what Spring's `HibernateTransactionManager` does: - **Joining.** `isExistingTransaction` is true while a transaction this manager began is in progress on the session the thread is using. `SessionHolder` records which of its sessions has one: not the holder as a whole, because `withNewSession` pushes a second session onto the same holder and a transaction there is its own; and not `getTransaction().isActive()`, because the simple map datastore's transaction reports active whether or not one is in progress. The mark is cleared once the transaction commits or rolls back, so transactional code run from its `afterCommit` or `afterCompletion` callbacks begins a transaction of its own instead of joining one that will not commit again. - **Commit and rollback** complete the transaction `doBegin` began (`TransactionObject.setTransaction`, a no-op until now, records it). A commit whose transaction is no longer active throws `IllegalTransactionStateException` instead of being skipped. A rollback of one does not: it follows a failed commit that has already ended the transaction, and throwing there would replace the commit failure (`doRollbackOnCommitException`). A session that began no transaction still has nothing to commit. - **`REQUIRES_NEW`** suspends the surrounding transaction and runs in a session of its own (`doSuspend` detaches the holder, `doBegin` opens a session when there is none). On resume, a session left bound by code that ran while suspended is closed, with a warning, instead of making the resume fail. - **A read-write transaction cannot join a read-only one**: the read-only commit would discard its writes. `validateExistingTransaction` is now on, so it fails at the join. - **A joined transaction that fails** clears the session as it marks the whole transaction rollback-only. Without a server-side transaction, a write left queued would otherwise be written by any explicit flush before the rollback, and could not be taken back. - **Cleanup** resets rollback-only, since a pre-bound holder outlives the transaction and the next one on it must not inherit the mark, and puts back the flush mode a read-only transaction changed. **A failed begin** rolls back what it started on a pre-bound session as well as on a new one; before, a refused timeout left a server transaction open on a pre-bound session, swallowing its next writes. GORM for MongoDB: - Beginning a transaction on a session whose server-side transaction is still active throws instead of aborting it. - A read-only transaction reads without a server-side transaction. It still refuses a per-transaction timeout, as a read-write one does. - The unified Spring Data manager suspends and resumes Spring Data's `MongoResourceHolder` with GORM's holder, so a `REQUIRES_NEW` transaction's `MongoTemplate` calls run in its own `ClientSession`. Its javadoc said `REQUIRES_NEW` and `NESTED` "join the surrounding transaction"; they aborted it. Neo4j keeps its own `isExistingTransaction`. Its `doBegin`, which replaces the base one, now opens a session of its own for `REQUIRES_NEW`, records the transaction it began, and records the flush mode it changes so cleanup puts it back. ## Behaviour change All documented in the 8.0 upgrade notes and the GORM for MongoDB guide (a new *Nested Transactions* section; *Read-Only Transactions* corrected, since the flush mode is now put back): - A nested transaction joins the surrounding one on MongoDB and the simple map datastore (`DataTest` unit tests): its writes are flushed at the outer commit, not when it returns. MongoDB does not flush before a query while a transaction is in progress, so a write a joined transaction left queued is not visible to later queries in the same transaction; it used to be. That includes `@Rollback` integration tests, whose service calls now join the test's transaction. - `REQUIRES_NEW` runs in a fresh session: entities loaded in the surrounding transaction and settings such as `withDatabase` do not carry over. - `NOT_SUPPORTED` suspends; `MANDATORY` inside a transaction works; `NEVER` and `NESTED` inside a transaction throw; a read-write transaction inside a read-only one throws, as does one declaring a different isolation level. - A commit whose transaction is no longer active throws. ## Tests - `NestedTransactionSpec` in the TCK, so every datastore runs it: join, inner failure, caught inner failure, `REQUIRES_NEW` isolation, and a transaction in `withNewSession`. It passes on MongoDB, Hibernate 5 and Hibernate 7, which is the parity this PR claims. Two features are `@PendingFeatureIf`, each for one datastore, and each fails the same way on `8.0.x`: Neo4j commits the writes made after a caught participant failure, and the simple map datastore loses a write made in `withNewSession`. - `DatastoreTransactionManagerSpec`: joining, rollback-only (with `TransactionTemplate` the outer commit throws `UnexpectedRollbackException`; with GORM's template, whose `inheritRollbackOnly` marks the outer transaction, it rolls back without one), synchronization timing, read-only inside read-write and the reverse, `REQUIRES_NEW`, `NOT_SUPPORTED`, a leftover session on resume, pre-bound sessions, flush-mode restore, an inactive transaction at commit, a failed commit reporting its own failure, a session that begins no transaction, and a failed begin. - `MongoTransactionSpec` (transactions on) and `MongoTransactionDisabledSpec` (off): the same against a real replica set, plus afterCommit, `withNewSession`, a refused begin on an active session, a refused timeout on a pre-bound session and on a read-only transaction. `MongoReadOnlyTransactionSpec`: a read-only transaction through a client that prefers secondaries. - `UnifiedMongoTransactionSpec` and `GormSharedSessionMongoTransactionManagerSpec`: joining and `REQUIRES_NEW` across GORM and `MongoTemplate`, and suspend/resume of Spring Data's holder. - Neo4j's `REQUIRES_NEW` propagation test is no longer `@Ignore`d; it passes. Run against an unmodified `8.0.x` checkout, the features for joining, propagation, read-only transactions and Spring Data's holder fail there. The others (a transaction in `withNewSession`, one run from `afterCommit`, a session that begins no transaction, a failed commit reporting its own error, a caught participant's queued writes, a read-only timeout, sequential transactions on a pre-bound session) pin behaviour `8.0.x` already has, which a narrower fix for joining breaks. Module suites, all passing: `grails-datastore-core` (294), `grails-data-mongodb-core` (921), `grails-data-mongodb-spring-data` (27), `grails-data-neo4j-core` (592), `grails-datamapping-core` (1333), `grails-datamapping-core-test` (632), `grails-data-hibernate5-core` (974), `grails-data-simple`, `grails-events-transforms` (whose `PublisherSpec` mocks a session that begins no transaction), and the TCK transaction specs on Hibernate 7. `codeStyle` is clean for every changed module. I have not run the whole `./gradlew build`. ## Related - Fixes #14391. - #15744 added server-side transactions and said a single flat transaction is supported; this makes that so. #16214 stopped read-only transactions from flushing; this stops them opening a server-side transaction, and puts back the flush mode they change. -- 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]
