Copilot commented on code in PR #24488:
URL: https://github.com/apache/camel/pull/24488#discussion_r3536768523
##########
core/camel-console/src/main/java/org/apache/camel/impl/console/JfrMemoryLeakDevConsole.java:
##########
@@ -137,7 +137,11 @@ private synchronized JsonObject doStart(Map<String,
Object> options) {
// trigger GC before starting to establish a cleaner baseline
System.gc();
try {
- Thread.sleep(500);
+ long deadline = System.currentTimeMillis() + 500;
+ long remaining;
+ while ((remaining = deadline - System.currentTimeMillis()) >
0) {
+ wait(remaining);
+ }
Review Comment:
`doStart()` is `synchronized`, so calling `wait(...)` here releases the
`this` monitor and allows concurrent `start/stop` requests to enter these
synchronized methods while the 500ms delay is running. That can race (e.g., two
concurrent `start` commands can both pass the `activeRecording == null` check
before either sets it), leading to overlapping recordings or inconsistent state.
To keep the mutual exclusion while still avoiding `Thread.sleep(...)` in a
synchronized method, wait on a private monitor object (so the `this` monitor is
not released) and use a monotonic clock for the deadline.
##########
core/camel-console/src/main/java/org/apache/camel/impl/console/JfrMemoryLeakDevConsole.java:
##########
@@ -202,7 +206,11 @@ private synchronized JsonObject
doStopRecordingAndParse(int limit) {
// trigger GC before stopping to flush objects into the recording
System.gc();
try {
- Thread.sleep(500);
+ long deadline = System.currentTimeMillis() + 500;
+ long remaining;
+ while ((remaining = deadline - System.currentTimeMillis()) >
0) {
+ wait(remaining);
+ }
Review Comment:
`doStopRecordingAndParse()` is `synchronized`, so the `wait(...)` call
releases the `this` monitor during the 500ms delay. That permits concurrent
`stop` calls to enter this method and potentially stop/dump the same
`Recording` concurrently, which can lead to failures or corrupted results.
Consider waiting on a private monitor object (so you don't release the
`this` lock that guards recording state) and use a monotonic clock for the
deadline.
##########
tooling/maven/camel-maven-plugin/src/main/java/org/apache/camel/maven/RunMojo.java:
##########
@@ -459,11 +459,13 @@ protected void afterBootstrapCamel() throws Exception {
// noop
}
- class IsolatedThreadGroup extends ThreadGroup {
+ class IsolatedThreadGroup implements Thread.UncaughtExceptionHandler {
+ @SuppressWarnings("java:S3014") // ThreadGroup is needed for thread
containment; no modern alternative in Java 17
+ private final ThreadGroup threadGroup;
Throwable uncaughtException; // synchronize access to this
IsolatedThreadGroup(String name) {
- super(name);
+ this.threadGroup = new ThreadGroup(name);
}
Review Comment:
`@SuppressWarnings("java:S3014")` is currently applied to the `ThreadGroup`
field, but the `new ThreadGroup(name)` instantiation in the constructor may
still be reported by Sonar as a separate S3014 hit. To ensure the rule is fully
suppressed for this unavoidable use, also apply the suppression to the
constructor (or the whole helper class).
--
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]
