You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@grails.apache.org by "borinquenkid (via GitHub)" <gi...@apache.org> on 2026/07/30 22:00:58 UTC
[PR] fix(neo4j): assert background thread completion in OptimisticLockingSpec [grails-core]
borinquenkid opened a new pull request, #16072:
URL: https://github.com/apache/grails-core/pull/16072
## Summary
- Follow-up to #16070 (merged). Copilot's review flagged that the second test's `Thread.start { ... }.join(2000)` can return on timeout without the background thread having actually finished, so the "same headroom rationale" comment added in #16070 was inaccurate there.
- Captures the thread and asserts `!isAlive()` after the bounded `join(5000)` so a slow runner fails loudly instead of silently racing the assertions that follow, instead of swallowing a potential `InterruptedException` and hoping for the best.
## Test plan
- [ ] CI run of `grails.gorm.tests.OptimisticLockingSpec` in `grails-data-neo4j` (could not execute this module's tests locally — it's pinned to `grails-gradle-plugins:6.1.2`, which 404s from `repo.grails.org` and isn't in local caches, a pre-existing environment gap unrelated to this change)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(neo4j): assert background thread completion in OptimisticLockingSpec [grails-core]
Posted by "jdaugherty (via GitHub)" <gi...@apache.org>.
jdaugherty commented on PR #16072:
URL: https://github.com/apache/grails-core/pull/16072#issuecomment-5815847062
I reviewed this since it was still set for RC2, but it looks like the upstream branch was changed. I'm going to remove this from RC2 and restore this PR as it was.
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(neo4j): assert background thread completion in OptimisticLockingSpec [grails-core]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #16072:
URL: https://github.com/apache/grails-core/pull/16072#issuecomment-5815854022
## 🚨 Jobs failed without executing tests 🚨
### Failed Jobs without Test Failures
❌ [CI / Hibernate7 Functional Tests \(Java 21, indy\=false, shard 2\)](https://github.com/apache/grails-core/actions/runs/32153101587/job/95763642101?pr=16072)
🏷️ Commit: 169193b0147747723d017b592a4584e83b3bf6f1
▶️ Tests: 0 executed
🟡 Checks: 74/76 completed
### Rerun Controls
Click the checkbox to trigger a rerun:
🔲 **Rerun jobs**
---
_Learn more about TestLens at [testlens.app/docs](https://testlens.app/docs/features/pr-comment/)._
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(neo4j): assert background thread completion in OptimisticLockingSpec [grails-core]
Posted by "jdaugherty (via GitHub)" <gi...@apache.org>.
jdaugherty commented on PR #16072:
URL: https://github.com/apache/grails-core/pull/16072#issuecomment-5319080188
@borinquenkid
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(neo4j): assert background thread completion in OptimisticLockingSpec [grails-core]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #16072:
URL: https://github.com/apache/grails-core/pull/16072#issuecomment-5136897106
## [Codecov](https://app.codecov.io/gh/apache/grails-core/pull/16072?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 52.1662%. Comparing base ([`184020c`](https://app.codecov.io/gh/apache/grails-core/commit/184020c5c1e93455fa3cf9e27103b05d3b8fe50b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`cd0502f`](https://app.codecov.io/gh/apache/grails-core/commit/cd0502f3bc84a3d62a50b0035b7be33867c36fe7?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/grails-core/pull/16072?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## feat/gorm-registry-core-impl #16072 +/- ##
======================================================================
+ Coverage 52.1558% 52.1662% +0.0104%
- Complexity 18408 18412 +4
======================================================================
Files 2054 2054
Lines 96622 96622
Branches 16846 16846
======================================================================
+ Hits 50394 50404 +10
+ Misses 38835 38825 -10
Partials 7393 7393
```
[see 3 files with indirect coverage changes](https://app.codecov.io/gh/apache/grails-core/pull/16072/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(neo4j): assert background thread completion in OptimisticLockingSpec [grails-core]
Posted by "jdaugherty (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(neo4j): assert background thread completion in OptimisticLockingSpec [grails-core]
Posted by "jdaugherty (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(neo4j): assert background thread completion in OptimisticLockingSpec [grails-core]
Posted by "jdaugherty (via GitHub)" <gi...@apache.org>.
jdaugherty commented on code in PR #16072:
URL: https://github.com/apache/grails-core/pull/16072#discussion_r4094479493
##########
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:
##########
@@ -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()
Review Comment:
The failure capture added below was only applied to the second test. This thread can still die silently (e.g. the `assert reloaded` on line 96), in which case the poll on line 107 times out after 10s reporting `observedName == 'locked'` and the actual exception is lost to stderr. Worth the same `AtomicReference<Throwable>` treatment for symmetry.
##########
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.
##########
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).
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(neo4j): assert background thread completion in OptimisticLockingSpec [grails-core]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #16072:
URL: https://github.com/apache/grails-core/pull/16072#issuecomment-5815820975
## 🔎 No tests executed 🔎
🏷️ Commit: 43fca1cf80bef4c36c8be23feaea74aff4d4fbb7
▶️ Tests: 0 executed
⚪️ Checks: 2/2 completed
---
_Learn more about TestLens at [testlens.app/docs](https://testlens.app/docs/features/pr-comment/)._
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(neo4j): assert background thread completion in OptimisticLockingSpec [grails-core]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #16072:
URL: https://github.com/apache/grails-core/pull/16072#issuecomment-5137249265
## ✅ All tests passed ✅
🏷️ Commit: cd0502f3bc84a3d62a50b0035b7be33867c36fe7
▶️ Tests: 40637 executed
⚪️ Checks: 55/55 completed
---
_Learn more about TestLens at [testlens.app](https://testlens.app)._
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(neo4j): assert background thread completion in OptimisticLockingSpec [grails-core]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #16072:
URL: https://github.com/apache/grails-core/pull/16072#issuecomment-5331443980
## ✅ All tests passed ✅
🏷️ Commit: 169193b0147747723d017b592a4584e83b3bf6f1
▶️ Tests: 45803 executed
⚪️ Checks: 70/70 completed
---
_Learn more about TestLens at [testlens.app](https://testlens.app)._
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(neo4j): make OptimisticLockingSpec commit, observe and verify the background write [grails-core]
Posted by "jdaugherty (via GitHub)" <gi...@apache.org>.
jdaugherty merged PR #16072:
URL: https://github.com/apache/grails-core/pull/16072
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org