(chores): fix SonarCloud S2276, S128, S3014 blocker issues#24488
(chores): fix SonarCloud S2276, S128, S3014 blocker issues#24488gnodet wants to merge 3 commits into
Conversation
- S2276: Replace Thread.sleep() with wait() in synchronized methods in JfrMemoryLeakDevConsole to release the monitor during sleep. Wrapped in while loop to guard against spurious wakeups. - S128: Add missing break statement in switch case fall-through in XmlRoutesBuilderLoader. - S3014: Refactor IsolatedThreadGroup in RunMojo to use composition with Thread.UncaughtExceptionHandler instead of extending ThreadGroup. Encapsulates ThreadGroup for thread containment. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
| long remaining; | ||
| while ((remaining = deadline - System.currentTimeMillis()) > 0) { | ||
| wait(remaining); | ||
| } |
There was a problem hiding this comment.
Hm, relying non monotonic clocks could be problematic. I think the Thread.sleep is safer in this case.
There was a problem hiding this comment.
Good catch — switched to Camel's StopWatch (backed by System.nanoTime(), monotonic) instead of System.currentTimeMillis() to avoid clock drift issues. Fixed in 859b1e0.
Claude Code on behalf of gnodet
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 18 tested, 29 compile-only — current: 18 all testedMaveniverse Scalpel detected 47 affected modules (current approach: 18).
|
Address review feedback: replace System.currentTimeMillis() with Camel's StopWatch (backed by System.nanoTime()) for the wait loop deadline, avoiding non-monotonic clock issues. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR targets SonarCloud blocker findings by adjusting thread-waiting behavior in a dev console, fixing an XML loader switch fall-through, and refactoring Maven plugin thread containment logic to reduce ThreadGroup exposure.
Changes:
- Replaced
Thread.sleep(500)inJfrMemoryLeakDevConsolewith a timedwait(...)loop insidesynchronizedmethods. - Added an explicit
break;inXmlRoutesBuilderLoaderto prevent switch fall-through. - Refactored
RunMojo’sIsolatedThreadGroupfrom inheritance (extends ThreadGroup) to composition plusThread.UncaughtExceptionHandler.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| core/camel-console/src/main/java/org/apache/camel/impl/console/JfrMemoryLeakDevConsole.java | Replaces Thread.sleep with timed waits during GC “settle” pauses in synchronized start/stop paths. |
| dsl/camel-xml-io-dsl/src/main/java/org/apache/camel/dsl/xml/io/XmlRoutesBuilderLoader.java | Adds missing break in a multi-label switch case. |
| tooling/maven/camel-maven-plugin/src/main/java/org/apache/camel/maven/RunMojo.java | Refactors thread grouping/exception capture to avoid extending ThreadGroup and centralize thread creation/management. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| long deadline = System.currentTimeMillis() + 500; | ||
| long remaining; | ||
| while ((remaining = deadline - System.currentTimeMillis()) > 0) { | ||
| wait(remaining); | ||
| } |
There was a problem hiding this comment.
Good catch — switched to a private gcWaitMonitor object so wait() does not release the this monitor. The synchronized(gcWaitMonitor) { gcWaitMonitor.wait(remaining); } pattern keeps the outer this lock held (preserving mutual exclusion for recording state) while using wait() instead of Thread.sleep() to satisfy S2276. Fixed in 4cbfa57.
Claude Code on behalf of gnodet
| long deadline = System.currentTimeMillis() + 500; | ||
| long remaining; | ||
| while ((remaining = deadline - System.currentTimeMillis()) > 0) { | ||
| wait(remaining); | ||
| } |
There was a problem hiding this comment.
Same fix as above — using a private gcWaitMonitor object to avoid releasing the this monitor. Fixed in 4cbfa57.
Claude Code on behalf of gnodet
| IsolatedThreadGroup(String name) { | ||
| super(name); | ||
| this.threadGroup = new ThreadGroup(name); | ||
| } |
There was a problem hiding this comment.
Moved @SuppressWarnings("java:S3014") to the class level so it covers both the field declaration and the new ThreadGroup(name) instantiation in the constructor. Fixed in 4cbfa57.
Claude Code on behalf of gnodet
- S2276: Use a private gcWaitMonitor object for wait() so that the 'this' monitor of synchronized methods is not released. This prevents race conditions (e.g., concurrent doStart() calls both passing the activeRecording == null check) while still avoiding Thread.sleep() inside a synchronized block. - S3014: Move @SuppressWarnings("java:S3014") from the field to the class level to cover both the field declaration and the constructor instantiation of ThreadGroup. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Fix 7 SonarCloud blocker issues across 3 rules:
S2276 —
wait()should be used instead ofThread.sleep()when a lock is held (2 issues)core/camel-console/src/main/java/org/apache/camel/impl/console/JfrMemoryLeakDevConsole.javaThread.sleep(500)withwait(500)in synchronized methodsdoStart()anddoStopRecordingAndParse().wait()overThread.sleep():Thread.sleep()inside asynchronizedblock holds the monitor lock for the entire sleep duration, blocking any other thread trying to enter the synchronized methods.wait()uses a separate monitor to avoid this pattern while still satisfying S2276.gcWaitMonitorobject for thewait()call so that the outerthismonitor (which guards recording state) is not released. This prevents race conditions (e.g., concurrentdoStart()calls both passing theactiveRecording == nullcheck).whileloop using Camel'sStopWatch(backed bySystem.nanoTime(), monotonic) to handle spurious wakeups and avoid non-monotonic clock issues.S128 — Switch cases should end with an unconditional break, return, or throw (1 issue)
dsl/camel-xml-io-dsl/src/main/java/org/apache/camel/dsl/xml/io/XmlRoutesBuilderLoader.javabreak;statement at the end of the multi-label case block to prevent fall-through to thedefaultcase.S3014 — Remove use of
ThreadGroup(4 issues)tooling/maven/camel-maven-plugin/src/main/java/org/apache/camel/maven/RunMojo.javaIsolatedThreadGroupfrom extendingThreadGroupto using composition withThread.UncaughtExceptionHandler. ThreadGroup is encapsulated internally (with class-level@SuppressWarnings) for thread containment since Java 17 has no modern alternative for thread enumeration by group. MovedgetActiveThreads()logic into the wrapper class and updatedjoinNonDaemonThreads()andterminateThreads()method signatures.Test plan
core/camel-console— 134 tests passdsl/camel-xml-io-dsl— 62 tests passtooling/maven/camel-maven-plugin— builds successfully (no test sources)Claude Code on behalf of gnodet
🤖 Generated with Claude Code