jdaugherty commented on code in PR #16072:
URL: https://github.com/apache/grails-core/pull/16072#discussion_r3793834256
##########
grails-data-neo4j/grails-datastore-gorm-neo4j/src/test/groovy/grails/gorm/tests/OptimisticLockingSpec.groovy:
##########
@@ -130,17 +130,18 @@ class OptimisticLockingSpec extends GormDatastoreSpec {
when:
o = OptLockNotVersioned.get(o.id)
- try {
- Thread.start {
- OptLockNotVersioned.withNewSession { s ->
- def reloaded = OptLockNotVersioned.get(o.id)
- reloaded.name += ' in new session'
- reloaded.save(flush: true)
- }
- }.join(2000)
- } catch (InterruptedException e) {
- // ignore
+ def backgroundUpdate = Thread.start {
+ OptLockNotVersioned.withNewSession { s ->
+ def reloaded = OptLockNotVersioned.get(o.id)
Review Comment:
This `get(o.id)` returns null on the background thread, so the closure dies
with an NPE on the next line before ever saving - silently, since nothing
observes the thread's exception. The node created in the `given:` block exists
only inside the main session's transaction: `GormDatastoreSpec.setup()` calls
`session.beginTransaction()` and nothing in this feature commits it, while the
background thread gets its own bolt session/transaction that cannot see the
uncommitted CREATE. The sibling test works around exactly this by committing
before spawning its thread (`session.transaction.commit();
session.transaction.nativeTransaction.close()`, lines 74-75). Without the same
commit here, the concurrent update this test is named for never happens, and
the new `join(5000)` + `isAlive()` check passes regardless.
##########
grails-data-neo4j/grails-datastore-gorm-neo4j/src/test/groovy/grails/gorm/tests/OptimisticLockingSpec.groovy:
##########
@@ -130,17 +130,18 @@ class OptimisticLockingSpec extends GormDatastoreSpec {
when:
o = OptLockNotVersioned.get(o.id)
- try {
- Thread.start {
- OptLockNotVersioned.withNewSession { s ->
- def reloaded = OptLockNotVersioned.get(o.id)
- reloaded.name += ' in new session'
- reloaded.save(flush: true)
- }
- }.join(2000)
- } catch (InterruptedException e) {
- // ignore
+ def backgroundUpdate = Thread.start {
+ OptLockNotVersioned.withNewSession { s ->
+ def reloaded = OptLockNotVersioned.get(o.id)
+ reloaded.name += ' in new session'
+ reloaded.save(flush: true)
+ }
}
+ // Unlike the unbounded join() above, join(timeout) can return before
the thread
+ // finishes; assert completion explicitly so a slow runner fails
loudly instead of
+ // silently racing the assertions below.
+ backgroundUpdate.join(5000)
+ assert !backgroundUpdate.isAlive()
Review Comment:
`assert !backgroundUpdate.isAlive()` verifies termination, not completion. A
thread whose closure throws is equally not-alive (the default
uncaught-exception handler just prints to stderr), so this assert passes when
the background update crashed - the same silent outcome the comment says it
prevents. And that crash path is live here (see the notes on lines 135 and
137). To actually assert completion, capture the closure's outcome and check it
on the test thread, e.g.:
```groovy
def failure = new AtomicReference<Throwable>()
def backgroundUpdate = Thread.start {
try {
OptLockNotVersioned.withNewSession {
def reloaded = OptLockNotVersioned.get(o.id)
assert reloaded
reloaded.name += ' in new session'
reloaded.save(flush: true)
}
} catch (Throwable t) {
failure.set(t)
}
}
backgroundUpdate.join(5000)
assert !backgroundUpdate.isAlive() && failure.get() == null
```
plus the `assert reloaded` guard the sibling test already has (line 93).
##########
grails-data-neo4j/grails-datastore-gorm-neo4j/src/test/groovy/grails/gorm/tests/OptimisticLockingSpec.groovy:
##########
@@ -130,17 +130,18 @@ class OptimisticLockingSpec extends GormDatastoreSpec {
when:
o = OptLockNotVersioned.get(o.id)
- try {
- Thread.start {
- OptLockNotVersioned.withNewSession { s ->
- def reloaded = OptLockNotVersioned.get(o.id)
- reloaded.name += ' in new session'
- reloaded.save(flush: true)
- }
- }.join(2000)
- } catch (InterruptedException e) {
- // ignore
+ def backgroundUpdate = Thread.start {
Review Comment:
Merge hazard: `feat/neo4j-gorm-registry-migration` (branched from the same
base commit) deletes this module and relocates this spec to
`grails-data-neo4j/core/src/test/groovy/grails/gorm/tests/OptimisticLockingSpec.groovy`
- still containing the old `join(2000)` block, plus a stray `session.` ->
`manager.session.` substitution inside the string literals (`' in new
manager.session'`). A `git merge-tree` of the two heads reports a content
conflict on this exact block, and the likely resolution (taking the migrated
file wholesale) silently drops this fix. Worth coordinating with that branch,
and/or applying the change to the TCK copy
(`grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/OptimisticLockingSpec.groovy`,
which still has `.join()` + `sleep(2000)`), which survives the migration.
##########
grails-data-neo4j/grails-datastore-gorm-neo4j/src/test/groovy/grails/gorm/tests/OptimisticLockingSpec.groovy:
##########
@@ -130,17 +130,18 @@ class OptimisticLockingSpec extends GormDatastoreSpec {
when:
o = OptLockNotVersioned.get(o.id)
- try {
- Thread.start {
- OptLockNotVersioned.withNewSession { s ->
- def reloaded = OptLockNotVersioned.get(o.id)
- reloaded.name += ' in new session'
- reloaded.save(flush: true)
- }
- }.join(2000)
- } catch (InterruptedException e) {
- // ignore
+ def backgroundUpdate = Thread.start {
+ OptLockNotVersioned.withNewSession { s ->
+ def reloaded = OptLockNotVersioned.get(o.id)
+ reloaded.name += ' in new session'
+ reloaded.save(flush: true)
Review Comment:
Even with the visibility problem fixed, this `save(flush: true)` is never
committed. Bare `withNewSession` only binds/unbinds a session; the flush runs
in a lazily started default transaction (`Neo4jSession.assertTransaction()` ->
`startDefaultTransaction()`), and on unbind `Neo4jSession.disconnect()` calls
`Neo4jTransaction.close()`, which closes the native bolt transaction without
committing - the driver rolls it back. The only committing path is
`Neo4jTransaction.commit()`, driven by `withTransaction`. The sibling test
wraps its background save in `OptLockVersioned.withTransaction { ... }` (line
91) for this reason; this closure needs the same (and the unused `s ->`
parameter can be dropped at the same time).
##########
grails-data-neo4j/grails-datastore-gorm-neo4j/src/test/groovy/grails/gorm/tests/OptimisticLockingSpec.groovy:
##########
@@ -130,17 +130,18 @@ class OptimisticLockingSpec extends GormDatastoreSpec {
when:
o = OptLockNotVersioned.get(o.id)
- try {
- Thread.start {
- OptLockNotVersioned.withNewSession { s ->
- def reloaded = OptLockNotVersioned.get(o.id)
- reloaded.name += ' in new session'
- reloaded.save(flush: true)
- }
- }.join(2000)
- } catch (InterruptedException e) {
- // ignore
+ def backgroundUpdate = Thread.start {
+ OptLockNotVersioned.withNewSession { s ->
+ def reloaded = OptLockNotVersioned.get(o.id)
+ reloaded.name += ' in new session'
+ reloaded.save(flush: true)
+ }
}
+ // Unlike the unbounded join() above, join(timeout) can return before
the thread
+ // finishes; assert completion explicitly so a slow runner fails
loudly instead of
+ // silently racing the assertions below.
Review Comment:
The assertions below cannot actually race the background update - they are
unfalsifiable by it. `o` is loaded before the thread starts (line 131, name
`locked`), line 148 appends ` in main session`, the save is a blind `SET n +=
$props` with no version predicate (`version false`), and the re-read at line
159 goes through the main session's still-open transaction, which sees its own
write (and holds the node's write lock, so a late background `SET` cannot land
between lines 151 and 159). Both `ex == null` and `o.name == 'locked in main
session'` hold even if lines 133-146 are deleted. Contrast the sibling's
`o.name == 'locked in new session'` (line 121), which does verify its
background write. Re-reading in a fresh session after the join and asserting
the name is `locked in new session` before the main save would make the
background update load-bearing and turn this into a real last-write-wins test.
##########
grails-data-neo4j/grails-datastore-gorm-neo4j/src/test/groovy/grails/gorm/tests/OptimisticLockingSpec.groovy:
##########
@@ -130,17 +130,18 @@ class OptimisticLockingSpec extends GormDatastoreSpec {
when:
o = OptLockNotVersioned.get(o.id)
- try {
- Thread.start {
- OptLockNotVersioned.withNewSession { s ->
- def reloaded = OptLockNotVersioned.get(o.id)
- reloaded.name += ' in new session'
- reloaded.save(flush: true)
- }
- }.join(2000)
- } catch (InterruptedException e) {
- // ignore
+ def backgroundUpdate = Thread.start {
+ OptLockNotVersioned.withNewSession { s ->
+ def reloaded = OptLockNotVersioned.get(o.id)
+ reloaded.name += ' in new session'
+ reloaded.save(flush: true)
+ }
}
+ // Unlike the unbounded join() above, join(timeout) can return before
the thread
+ // finishes; assert completion explicitly so a slow runner fails
loudly instead of
+ // silently racing the assertions below.
+ backgroundUpdate.join(5000)
+ assert !backgroundUpdate.isAlive()
// Same headroom rationale as "Test optimistic locking" above.
sleep 5000
Review Comment:
Optional, pre-existing: this fixed `sleep 5000` (and the sibling's at line
103) costs 5s on every green run. Once the background write actually lands (see
the notes above), the deterministic replacement is a
`spock.util.concurrent.PollingConditions` poll on the observable state -
already the idiom in `DirtyCheckingAfterListenerSpec` and
`HibernateUpdateFromListenerSpec`, and spock-core is on this module's test
classpath. It returns as soon as the write is visible and fails with the actual
observed value on timeout, instead of sleeping a fixed budget and hoping.
##########
grails-data-neo4j/grails-datastore-gorm-neo4j/src/test/groovy/grails/gorm/tests/OptimisticLockingSpec.groovy:
##########
@@ -130,17 +130,18 @@ class OptimisticLockingSpec extends GormDatastoreSpec {
when:
o = OptLockNotVersioned.get(o.id)
- try {
- Thread.start {
- OptLockNotVersioned.withNewSession { s ->
- def reloaded = OptLockNotVersioned.get(o.id)
- reloaded.name += ' in new session'
- reloaded.save(flush: true)
- }
- }.join(2000)
- } catch (InterruptedException e) {
- // ignore
+ def backgroundUpdate = Thread.start {
+ OptLockNotVersioned.withNewSession { s ->
+ def reloaded = OptLockNotVersioned.get(o.id)
+ reloaded.name += ' in new session'
+ reloaded.save(flush: true)
+ }
}
+ // Unlike the unbounded join() above, join(timeout) can return before
the thread
+ // finishes; assert completion explicitly so a slow runner fails
loudly instead of
+ // silently racing the assertions below.
+ backgroundUpdate.join(5000)
Review Comment:
This converts a previously non-failing wait into a hard 5s cliff. Before, a
background thread finishing in more than 2s was tolerated: `join(2000)` returns
silently on timeout and the following `sleep 5000` absorbed the overrun (~7s of
effective grace). Now a single run over 5s fails at line 144, and the `sleep
5000` headroom sits after the assert, so it no longer contributes anything to
the completion check. With `grails-data-neo4j/build.gradle` setting `retry {
maxRetries = 2; failOnPassedAfterRetry = true }`, one slow run reds the build
even when the retry passes. Every equivalent test in the tree - the sibling
above (line 98), the TCK copy in `grails-datamapping-tck`, and the hibernate5/7
variants - uses an unbounded `.join()`, which cannot return early and has no
cliff; converging on that is both simpler and stronger. If the bounded form
stays, consider a larger budget and an assert message that includes
`backgroundUpdate.state` so a CI-only timeout is triageable.
--
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]
