sergehuber commented on code in PR #749: URL: https://github.com/apache/unomi/pull/749#discussion_r2789894426
########## lifecycle-watcher/src/main/java/org/apache/unomi/lifecycle/BundleWatcherImpl.java: ########## Review Comment: `cancel(true)` should be changed to `cancel(false)` here. To see why, trace the call path: 1. `checkStartupComplete()` calls `startScheduler(getBundleCheckTask())`, which schedules a recurring task via `scheduleWithFixedDelay`. 2. When that task fires, it runs `getBundleCheckTask().run()`, which calls back into `checkStartupComplete()`. 3. `checkStartupComplete()` now starts with `destroyScheduler()`, which calls `scheduledFuture.cancel(true)`. 4. At this point, the thread calling `cancel(true)` *is* the thread running the scheduled task — it is cancelling itself. The `true` argument means "interrupt if running", which sets the current thread's interrupt flag. The thread then continues executing the rest of `checkStartupComplete()` with its interrupt flag set. Any subsequent interruptible operation in that same execution — in particular, NIO-based log appenders writing to a `FileChannel` — may then throw `ClosedByInterruptException`, which permanently closes the channel and can break logging for the rest of the process lifetime. This is hard to reproduce because it depends on timing and on which logging backend and appenders are configured. Using `cancel(false)` prevents future scheduled executions without interrupting the current one, which is the intended behavior here. References: - [JavaDoc — Future.cancel(boolean)](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/concurrent/Future.html#cancel(boolean)) ("if the task has already started, then the `mayInterruptIfRunning` parameter determines whether the thread executing this task should be interrupted") - [JavaDoc — ClosedByInterruptException](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/nio/channels/ClosedByInterruptException.html) ("thrown when a thread is interrupted while blocked in an I/O operation upon a channel — the channel is closed as a side-effect") ########## lifecycle-watcher/src/main/java/org/apache/unomi/lifecycle/BundleWatcherImpl.java: ########## @@ -58,6 +58,9 @@ public class BundleWatcherImpl implements SynchronousBundleListener, ServiceList private List<ServerInfo> serverInfos = new ArrayList<>(); + // Lock object to synchronize startup message display + private final Object startupMessageLock = new Object(); Review Comment: `startupMessageAlreadyDisplayed` should be declared `volatile` here, because it is read *outside* the `synchronized` block (line 290) as part of a double-checked locking pattern. Without `volatile`, the Java Memory Model allows a thread to read a stale cached value indefinitely — meaning a thread could keep seeing `false` even after another thread has set it to `true` inside the lock. In practice, stale reads are rare on strongly-ordered architectures like x86 (which is why the bug may not show up during development), but they are much more frequent on weakly-ordered architectures like ARM — commonly used in cloud/container deployments. Adding `volatile` guarantees that every write is immediately visible to all threads, which is what makes the outer check reliable. References: - [CERT Oracle Coding Standard — LCK10-J: Use a correct form of the double-checked locking idiom](https://wiki.sei.cmu.edu/confluence/display/java/LCK10-J.+Use+a+correct+form+of+the+double-checked+locking+idiom) - [Oracle Java Tutorial — Atomic Access](https://docs.oracle.com/javase/tutorial/essential/concurrency/atomic.html) - [JLS §17.4 — Memory Model](https://docs.oracle.com/javase/specs/jls/se17/html/jls-17.html#jls-17.4) (defines the happens-before rules that govern when writes become visible across threads) -- 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]
