You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by "paulk-asert (via GitHub)" <gi...@apache.org> on 2026/09/06 05:38:52 UTC
[PR] GROOVY-12371: bound the Sql statement cache and make it thread-safe [groovy]
paulk-asert opened a new pull request, #2895:
URL: https://github.com/apache/groovy/pull/2895
The prepared-statement cache was an unbounded HashMap keyed on the SQL text, mutated from getAbstractStatement without synchronization. Two problems followed. It grew without limit whenever the text varied rather than the parameters — an inList expands to a different placeholder count per list size, so a caller sizing the list mints a distinct cached statement each time, held open until the Sql is closed. And a shared Sql using the cache from more than one thread could corrupt the map.
The cache is now a synchronized, access-ordered LinkedHashMap that evicts the least recently used statement past a cap, closing it as it goes so a bounded cache does not leak the cursor it drops. The cap is statementCacheSize, default 256, settable and overridable with the groovy.sql.statement.cache.size system property; 0 or less keeps the old unbounded behaviour for anyone who relied on it.
Statement creation stays outside the lock — only the get-then-put is synchronized — so preparing a statement does not serialize other callers, and a create race keeps the first result and closes the loser rather than leaking it.
One consequence to note: with a shared Sql caching across threads, a bound below the working set can now evict and close a statement another thread is about to reuse. A single Sql is typically used serially or per connection; where it is shared, size the cache above the working set (or leave it unbounded). Default 256 is generous enough that ordinary use never evicts.
--
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@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12371: bound the Sql statement cache and make it thread-safe [groovy]
Posted by "paulk-asert (via GitHub)" <gi...@apache.org>.
paulk-asert commented on code in PR #2895:
URL: https://github.com/apache/groovy/pull/2895#discussion_r3943889193
##########
subprojects/groovy-sql/src/main/java/groovy/sql/Sql.java:
##########
@@ -5057,8 +5111,18 @@ private Statement getAbstractStatement(AbstractStatementCommand cmd, Connection
if (cacheStatements) {
stmt = statementCache.get(sql);
if (stmt == null) {
- stmt = cmd.execute(connection, sql);
- statementCache.put(sql, stmt);
+ Statement created = cmd.execute(connection, sql);
+ synchronized (statementCache) {
+ Statement raced = statementCache.get(sql);
Review Comment:
I think either approach is fine in terms of pros/cons.
> DCL moves cmd.execute inside the lock. That JDBC prepare is a DB round-trip, and Collections.synchronizedMap uses one mutex for the whole map — so while one thread prepares a statement under the lock, every other thread's cache lookup and every other prepare — even of completely different SQL — blocks on that same mutex. DCL serializes all statement preparation across a shared Sql.
>
> My version deliberately never holds the mutex across the DB call, so unrelated prepares proceed in parallel. The cost is exactly what Daniel spotted: a rare double-create.
>
> So it's a genuine trade:
> - Mine: parallel prepares; a rare wasted prepare when two threads hit the same brand-new SQL simultaneously.
> - His DCL: never double-creates; but serializes all preparation behind one lock.
--
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@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12371: bound the Sql statement cache and make it thread-safe [groovy]
Posted by "paulk-asert (via GitHub)" <gi...@apache.org>.
paulk-asert merged PR #2895:
URL: https://github.com/apache/groovy/pull/2895
--
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@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12371: bound the Sql statement cache and make it thread-safe [groovy]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #2895:
URL: https://github.com/apache/groovy/pull/2895#issuecomment-5557402286
## [Codecov](https://app.codecov.io/gh/apache/groovy/pull/2895?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `80.00000%` with `6 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 71.0797%. Comparing base ([`5a64fb2`](https://app.codecov.io/gh/apache/groovy/commit/5a64fb2ca5232fbc3f39993bb759afc4c0c7ab48?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`cade0cb`](https://app.codecov.io/gh/apache/groovy/commit/cade0cb3eda786eaf5b6afc41024ef8c067323c3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/groovy/pull/2895?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...jects/groovy-sql/src/main/java/groovy/sql/Sql.java](https://app.codecov.io/gh/apache/groovy/pull/2895?src=pr&el=tree&filepath=subprojects%2Fgroovy-sql%2Fsrc%2Fmain%2Fjava%2Fgroovy%2Fsql%2FSql.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VicHJvamVjdHMvZ3Jvb3Z5LXNxbC9zcmMvbWFpbi9qYXZhL2dyb292eS9zcWwvU3FsLmphdmE=) | 80.0000% | [4 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/groovy/pull/2895?src=pr&el=tree&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/groovy/pull/2895?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #2895 +/- ##
==================================================
- Coverage 71.0806% 71.0797% -0.0009%
- Complexity 37330 37333 +3
==================================================
Files 1579 1579
Lines 135480 135500 +20
Branches 25105 25108 +3
==================================================
+ Hits 96300 96313 +13
- Misses 30496 30497 +1
- Partials 8684 8690 +6
```
| [Files with missing lines](https://app.codecov.io/gh/apache/groovy/pull/2895?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...jects/groovy-sql/src/main/java/groovy/sql/Sql.java](https://app.codecov.io/gh/apache/groovy/pull/2895?src=pr&el=tree&filepath=subprojects%2Fgroovy-sql%2Fsrc%2Fmain%2Fjava%2Fgroovy%2Fsql%2FSql.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VicHJvamVjdHMvZ3Jvb3Z5LXNxbC9zcmMvbWFpbi9qYXZhL2dyb292eS9zcWwvU3FsLmphdmE=) | `67.4107% <80.0000%> (+0.2289%)` | :arrow_up: |
... and [10 files with indirect coverage changes](https://app.codecov.io/gh/apache/groovy/pull/2895/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@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12371: bound the Sql statement cache and make it thread-safe [groovy]
Posted by "daniellansun (via GitHub)" <gi...@apache.org>.
daniellansun commented on PR #2895:
URL: https://github.com/apache/groovy/pull/2895#issuecomment-5560001521
LGTM
--
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@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12371: bound the Sql statement cache and make it thread-safe [groovy]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #2895:
URL: https://github.com/apache/groovy/pull/2895#issuecomment-5557533690
## ✅ All tests passed ✅
🏷️ Commit: cade0cb3eda786eaf5b6afc41024ef8c067323c3
▶️ Tests: 116788 executed
⚪️ Checks: 32/32 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@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12371: bound the Sql statement cache and make it thread-safe [groovy]
Posted by "daniellansun (via GitHub)" <gi...@apache.org>.
daniellansun commented on code in PR #2895:
URL: https://github.com/apache/groovy/pull/2895#discussion_r3943648072
##########
subprojects/groovy-sql/src/main/java/groovy/sql/Sql.java:
##########
@@ -5057,8 +5111,18 @@ private Statement getAbstractStatement(AbstractStatementCommand cmd, Connection
if (cacheStatements) {
stmt = statementCache.get(sql);
if (stmt == null) {
- stmt = cmd.execute(connection, sql);
- statementCache.put(sql, stmt);
+ Statement created = cmd.execute(connection, sql);
+ synchronized (statementCache) {
+ Statement raced = statementCache.get(sql);
Review Comment:
Why not get and check the `raced` outside of `syncronized` block first and use double-checked locking to avoid the statement with the same SQL is created repeatedly.
--
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@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12371: bound the Sql statement cache and make it thread-safe [groovy]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #2895:
URL: https://github.com/apache/groovy/pull/2895#issuecomment-5560029056
## 🚨 TestLens detected 1 failed test 🚨
Here is what you can do:
1) Inspect the test failures carefully.
2) If you are convinced that some of the tests are flaky, you can mute them below.
3) Finally, trigger a rerun by checking the rerun checkbox.
### Test Summary
#### [Build and test / lts \(17, windows-latest, 1\)](https://github.com/apache/groovy/actions/runs/34039264166/job/101503134838?pr=2895) > :groovy-groovydoc:test
| Test | Runs | Flakiness |
|---|---|--:|
| GroovyDocToolTest > testDocFilesSymlinkIsNotCopiedIntoOutput | ❌ | 13% 🔴 |
🏷️ Commit: 2c20bde804f94e44af4d4023369e5a22e539e06e
▶️ Tests: 64373 executed
🟡 Checks: 10/30 completed
### Test Failures
<details><summary><strong>GroovyDocToolTest > testDocFilesSymlinkIsNotCopiedIntoOutput</strong> (:groovy-groovydoc:test in <a href="https://github.com/apache/groovy/actions/runs/34039264166/job/101503134838?pr=2895">Build and test / lts (17, windows-latest, 1)</a>)</summary>
```
junit.framework.ComparisonFailure: a real doc-files asset should still be copied expected:<a real asset> but was:<null>
at junit.framework.Assert.assertEquals(Assert.java:100)
at junit.framework.TestCase.assertEquals(TestCase.java:253)
at org.codehaus.groovy.tools.groovydoc.GroovyDocToolTest.testDocFilesSymlinkIsNotCopiedIntoOutput(GroovyDocToolTest.java:438)
```
</details>
### Rerun Controls
> [!NOTE]
> Checks are currently running using the configuration below.
Select tests to mute in this pull request:
🔲 GroovyDocToolTest > testDocFilesSymlinkIsNotCopiedIntoOutput <!-- uniqueId=[engine:junit-vintage]/[runner:org.codehaus.groovy.tools.groovydoc.GroovyDocToolTest]/[test:testDocFilesSymlinkIsNotCopiedIntoOutput(org.codehaus.groovy.tools.groovydoc.GroovyDocToolTest)] -->
Reuse successful test results:
🔲 ♻️ Only rerun the tests that failed or were muted before
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@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12371: bound the Sql statement cache and make it thread-safe [groovy]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #2895:
URL: https://github.com/apache/groovy/pull/2895#issuecomment-5562075437
## 🚨 TestLens detected 1 failed test 🚨
Here is what you can do:
1) Inspect the test failures carefully.
2) If you are convinced that some of the tests are flaky, you can mute them below.
3) Finally, trigger a rerun by checking the rerun checkbox.
### Test Summary
#### [Build and test / lts \(17, windows-latest, 1\)](https://github.com/apache/groovy/actions/runs/34057357048/job/101551573212?pr=2895) > :test
| Test | Runs | Flakiness |
|---|---|--:|
| BroadcastChannelAsPublisherTest > forAwaitOverPublisher\(\) | ❌ | 1% 🟡 |
🏷️ Commit: 6465647b1931b98138df5af72c0752bfea5347a9
▶️ Tests: 105459 executed
🟡 Checks: 22/30 completed
### Test Failures
<details><summary><strong>BroadcastChannelAsPublisherTest > forAwaitOverPublisher()</strong> (:test in <a href="https://github.com/apache/groovy/actions/runs/34057357048/job/101551573212?pr=2895">Build and test / lts (17, windows-latest, 1)</a>)</summary>
```
groovy.concurrent.ChannelClosedException: BroadcastChannel is closed
at groovy.concurrent.BroadcastChannel.subscribe(BroadcastChannel.java:97)
at groovy.concurrent.BroadcastChannel.subscribe(BroadcastChannel.java:87)
at groovy.concurrent.BroadcastChannel$BroadcastFlowPublisher.subscribe(BroadcastChannel.java:200)
at org.apache.groovy.runtime.async.FlowPublisherAdapter$PublisherBlockingIterable.iterator(FlowPublisherAdapter.java:169)
at TestScript2.run(TestScript2.groovy:14)
at groovy.lang.GroovyShell.evaluate(GroovyShell.java:552)
at groovy.lang.GroovyShell.evaluate(GroovyShell.java:587)
at groovy.lang.GroovyShell.evaluate(GroovyShell.java:571)
at groovy.test.GroovyAssert.assertScript(GroovyAssert.java:107)
at groovy.test.GroovyAssert.assertScript(GroovyAssert.java:97)
at groovy.concurrent.BroadcastChannelAsPublisherTest.forAwaitOverPublisher(BroadcastChannelAsPublisherTest.groovy:203)
```
</details>
### Rerun Controls
> [!NOTE]
> Checks are currently running using the configuration below.
Select tests to mute in this pull request:
🔲 BroadcastChannelAsPublisherTest > forAwaitOverPublisher\(\) <!-- uniqueId=[engine:junit-jupiter]/[class:groovy.concurrent.BroadcastChannelAsPublisherTest]/[method:forAwaitOverPublisher()] -->
Reuse successful test results:
🔲 ♻️ Only rerun the tests that failed or were muted before
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@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12371: bound the Sql statement cache and make it thread-safe [groovy]
Posted by "daniellansun (via GitHub)" <gi...@apache.org>.
daniellansun commented on code in PR #2895:
URL: https://github.com/apache/groovy/pull/2895#discussion_r3944295194
##########
subprojects/groovy-sql/src/main/java/groovy/sql/Sql.java:
##########
@@ -5057,8 +5111,18 @@ private Statement getAbstractStatement(AbstractStatementCommand cmd, Connection
if (cacheStatements) {
stmt = statementCache.get(sql);
if (stmt == null) {
- stmt = cmd.execute(connection, sql);
- statementCache.put(sql, stmt);
+ Statement created = cmd.execute(connection, sql);
+ synchronized (statementCache) {
+ Statement raced = statementCache.get(sql);
Review Comment:
Understood
--
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@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org