Skip to content

Commit aa8ac88

Browse files
csviriCopilot
andauthored
fix: create the PollingEventSource timer on start and make it a daemon (#3523)
`PollingEventSource` held its timer in a final field initialised at construction: private final Timer timer = new Timer(); Two problems follow from that. The event source cannot be restarted. `stop()` calls `timer.cancel()` and a cancelled `java.util.Timer` cannot be reused, so a subsequent `start()` fails with `IllegalStateException: Timer already cancelled`. Restart is a supported lifecycle - `Operator.stop()` / `start()` recreates the thread pools for exactly this reason, and `TimerEventSource` creates a new `Timer` inside `start()`. The timer thread is not a daemon and is created eagerly. Merely constructing a `PollingEventSource` therefore starts a non-daemon thread that keeps the JVM from exiting, even if the event source is never started, and it outlives an operator that is stopped without stopping its event sources. `TimerEventSource` uses `new Timer(true)`. The timer is now created in `start()` as a daemon and cleared in `stop()`, matching `TimerEventSource`. Adds regression tests for restart and for the daemon flag; the restart one fails with `IllegalStateException: Timer already cancelled` without this change. Note: `PerResourcePollingEventSource` has a related restart limitation because `stop()` calls `shutdownNow()` on the `ScheduledExecutorService` from its configuration. That executor is supplied by the caller, so changing its ownership semantics is a separate discussion and is left out of this change. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
1 parent 6ff8b15 commit aa8ac88

2 files changed

Lines changed: 41 additions & 2 deletions

File tree

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingEventSource.java

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ public class PollingEventSource<R, P extends HasMetadata, ID>
6262

6363
private static final Logger log = LoggerFactory.getLogger(PollingEventSource.class);
6464

65-
private final Timer timer = new Timer();
65+
private Timer timer;
6666
private final GenericResourceFetcher<R> genericResourceFetcher;
6767
private final Duration period;
6868
private final AtomicBoolean healthy = new AtomicBoolean(true);
@@ -75,8 +75,12 @@ public PollingEventSource(Class<R> resourceClass, PollingConfiguration<R, ID> co
7575

7676
@Override
7777
public void start() throws OperatorException {
78+
if (timer != null) {
79+
return;
80+
}
7881
super.start();
7982
getStateAndFillCache();
83+
timer = new Timer(true);
8084
timer.schedule(
8185
new TimerTask() {
8286
@Override
@@ -111,7 +115,10 @@ public interface GenericResourceFetcher<R> {
111115
@Override
112116
public void stop() throws OperatorException {
113117
super.stop();
114-
timer.cancel();
118+
if (timer != null) {
119+
timer.cancel();
120+
timer = null;
121+
}
115122
}
116123

117124
@Override

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingEventSourceTest.java

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,38 @@ public void setup() {
6060
setUpSource(pollingEventSource, false);
6161
}
6262

63+
@Test
64+
void canBeRestartedAfterStop() throws InterruptedException {
65+
when(resourceFetcher.fetchResources()).thenReturn(testResponseWithTwoValues());
66+
pollingEventSource.start();
67+
Thread.sleep(DEFAULT_WAIT_PERIOD);
68+
pollingEventSource.stop();
69+
70+
// a cancelled java.util.Timer cannot be reused, a new one has to be created on start
71+
pollingEventSource.start();
72+
Thread.sleep(DEFAULT_WAIT_PERIOD);
73+
74+
assertThat(pollingEventSource.getStatus()).isEqualTo(Status.HEALTHY);
75+
}
76+
77+
@Test
78+
void timerThreadIsADaemonSoItDoesNotKeepTheJvmAlive() throws InterruptedException {
79+
when(resourceFetcher.fetchResources()).thenReturn(testResponseWithTwoValues());
80+
81+
var threadsBeforeStart = Thread.getAllStackTraces().keySet();
82+
83+
pollingEventSource.start();
84+
Thread.sleep(DEFAULT_WAIT_PERIOD);
85+
86+
var newTimerThreads =
87+
Thread.getAllStackTraces().keySet().stream()
88+
.filter(t -> t.getName().startsWith("Timer-"))
89+
.filter(t -> !threadsBeforeStart.contains(t))
90+
.toList();
91+
92+
assertThat(newTimerThreads).isNotEmpty().allMatch(Thread::isDaemon);
93+
}
94+
6395
@Test
6496
void pollsAndProcessesEvents() throws InterruptedException {
6597
when(resourceFetcher.fetchResources()).thenReturn(testResponseWithTwoValues());

0 commit comments

Comments
 (0)