jdaugherty commented on code in PR #16072:
URL: https://github.com/apache/grails-core/pull/16072#discussion_r4094693177
##########
grails-data-neo4j/grails-datastore-gorm-neo4j/src/test/groovy/grails/gorm/tests/OptimisticLockingSpec.groovy:
##########
@@ -125,24 +135,41 @@ class OptimisticLockingSpec extends GormDatastoreSpec {
given:
def o = new OptLockNotVersioned(name: 'locked').save(flush: true)
+ session.transaction.commit()
+ session.transaction.nativeTransaction.close()
session.clear()
when:
o = OptLockNotVersioned.get(o.id)
- try {
- Thread.start {
+ def failure = new AtomicReference<Throwable>()
+ def backgroundUpdate = Thread.start {
+ try {
OptLockNotVersioned.withNewSession { s ->
- def reloaded = OptLockNotVersioned.get(o.id)
- reloaded.name += ' in new session'
- reloaded.save(flush: true)
+ OptLockNotVersioned.withTransaction {
+ def reloaded = OptLockNotVersioned.get(o.id)
+ assert reloaded
+ reloaded.name += ' in new session'
+ reloaded.save(flush: true)
+ }
}
- }.join(2000)
- } catch (InterruptedException e) {
- // ignore
+ } catch (Throwable t) {
+ failure.set(t)
+ }
+ }
+ backgroundUpdate.join()
+ // A thread that dies from an uncaught exception is also no longer
alive, so join()
+ // alone can't distinguish a completed write from a crashed one;
assert the captured
+ // outcome explicitly.
+ assert failure.get() == null
Review Comment:
Nit: on failure the power assert renders only `failure.get()`'s
`toString()`, so the background thread's stack trace is lost. `if
(failure.get()) throw failure.get()` keeps the full trace in the report.
##########
grails-data-neo4j/grails-datastore-gorm-neo4j/src/test/groovy/grails/gorm/tests/OptimisticLockingSpec.groovy:
##########
@@ -96,11 +99,18 @@ class OptimisticLockingSpec extends GormDatastoreSpec {
}
}
}.join()
- // The background thread's save is already synchronized via join()
above; this sleep is
- // headroom for the embedded Neo4j harness's own write durability, not
thread completion.
- // A noisy/loaded CI runner can push that past a couple of seconds -
give it more room
- // rather than risk a spurious failure (heisenbug).
- sleep 5000
+ // The background thread's save is already synchronized via join()
above; poll (rather
+ // than sleep a fixed duration) until an independent session observes
it, since the
+ // embedded Neo4j harness's own write-durability/visibility lag can
outlast any fixed
+ // guess (heisenbug) - a noisy/loaded CI runner has been seen pushing
past 2s, and this
+ // adapts instead of gambling on a bigger number.
Review Comment:
This rationale does not match what actually failed. The CI failure that
motivated #16070 (run 29704447551 on #15972, all three Neo4j legs) was
`ConditionNotSatisfiedError at OptimisticLockingSpec.groovy:102`. At that
commit, line 102 of *this* file is `o.name += ' in main session'`, which is not
a condition, while line 102 of the TCK copy
(`grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/OptimisticLockingSpec.groovy`,
which `grails-data-neo4j-core:test` also runs via the extracted TCK classes)
is `ex instanceof OptimisticLockingException`. That TCK test is the one that
was failing; #15972 later diagnosed it as the uncommitted-node / read-committed
problem and `@IgnoreIf`'d it for Neo4j. No run of this file has ever been
observed outlasting a sleep, and a Neo4j commit is synchronous: once `join()`
returns, the write is visible to any new transaction, so there is no durability
or visibility lag to adapt to.
Keeping the poll as a cheap guard is fine, but the comment should not assert
a mechanism that is not there. Something like "poll until an independent
session sees the write rather than sleep a fixed budget" is enough, and the
"seen pushing past 2s" claim should go (same for the "visibility-lag rationale"
reference on line 165).
##########
grails-data-neo4j/grails-datastore-gorm-neo4j/src/test/groovy/grails/gorm/tests/OptimisticLockingSpec.groovy:
##########
@@ -96,11 +99,18 @@ class OptimisticLockingSpec extends GormDatastoreSpec {
}
}
}.join()
- // The background thread's save is already synchronized via join()
above; this sleep is
- // headroom for the embedded Neo4j harness's own write durability, not
thread completion.
- // A noisy/loaded CI runner can push that past a couple of seconds -
give it more room
- // rather than risk a spurious failure (heisenbug).
- sleep 5000
+ // The background thread's save is already synchronized via join()
above; poll (rather
+ // than sleep a fixed duration) until an independent session observes
it, since the
+ // embedded Neo4j harness's own write-durability/visibility lag can
outlast any fixed
+ // guess (heisenbug) - a noisy/loaded CI runner has been seen pushing
past 2s, and this
+ // adapts instead of gambling on a bigger number.
+ new PollingConditions(timeout: 10, initialDelay: 0.1, delay:
0.2).eventually {
+ def observedName
+ OptLockVersioned.withNewSession { s ->
Review Comment:
Nit, carried over from the previous round: the `s ->` parameters are still
unused in all four `withNewSession` closures (lines 93, 109, 148, 168).
##########
grails-data-neo4j/grails-datastore-gorm-neo4j/src/test/groovy/grails/gorm/tests/OptimisticLockingSpec.groovy:
##########
@@ -125,24 +135,41 @@ class OptimisticLockingSpec extends GormDatastoreSpec {
given:
def o = new OptLockNotVersioned(name: 'locked').save(flush: true)
+ session.transaction.commit()
+ session.transaction.nativeTransaction.close()
Review Comment:
Nit: `Neo4jTransaction.commit()` already closes the native transaction and
its bolt session (`commit()` calls `close()`), so this
`nativeTransaction.close()` is a no-op copied from line 78. Harmless, but it
reads as if something were still open.
--
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]
