SLING-13170 Replace Timer with injectable ScheduledExecutorService#55
Open
daniancu wants to merge 5 commits into
Open
SLING-13170 Replace Timer with injectable ScheduledExecutorService#55daniancu wants to merge 5 commits into
daniancu wants to merge 5 commits into
Conversation
added 3 commits
April 14, 2026 15:48
Replace the fire-and-forget Timer in startProcessing() with a ScheduledExecutorService that is properly shut down on deactivate and can be replaced in tests via setScheduler(). This eliminates the flaky Thread.sleep(4000) in testTopologyChange by using a ManualScheduler that captures tasks for on-demand execution, making the test deterministic and ~8x faster. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Ai-Assisted-By: claude
- Use AtomicReference for thread-safe scheduler access - Guard startProcessing() against null scheduler and RejectedExecutionException when shutdown races with task submission - Model realistic shutdown semantics in ManualScheduler test fake - Add testDeactivatePreventsDelayedNotification covering deactivation clearing pending tasks and schedule/shutdown race safety Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Ai-Assisted-By: claude
…ivate - testStartProcessingWithNullScheduler: covers the scheduler.get()==null branch when deactivate() has already cleared the AtomicReference - testDeactivateWithNullScheduler: covers deactivate() when scheduler was never initialized or already cleared (including double-deactivate) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Ai-Assisted-By: claude
joerghoh
reviewed
Apr 15, 2026
| * and clears pending tasks; {@link #schedule} after shutdown throws {@link RejectedExecutionException}; | ||
| * {@link #runAll()} skips execution after shutdown. | ||
| */ | ||
| private static class ManualScheduler implements ScheduledExecutorService { |
Contributor
There was a problem hiding this comment.
I would externalize this internal class into a dedicated class.
|
|
||
| /** | ||
| * Replace the scheduler used for delayed topology change processing. | ||
| * Package-private for testability. |
There was a problem hiding this comment.
This is an OSGi component, why would this setScheduler be needed, if simply another could be injected (if it was made so that it would be an OSGi service as well)?
Author
There was a problem hiding this comment.
it's just a backdoor for testing since tests don't use OSGi. in production we do not need to inject, it runs with the default one
Move ManualScheduler from a private inner class in JobManagerConfigurationTest to a standalone package-private class for better readability. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace the package-private setScheduler() setter with reflection-based injection in tests, consistent with how startupDelayListener is already injected. The scheduler AtomicReference is now only accessed directly by production code. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Ai-Assisted-By: claude
Author
Changes since initial PRCommit 2 — Harden scheduler lifecycle
Commit 3 — Null scheduler tests
Commit 4 — Extract ManualScheduler
Commit 5 — Remove setScheduler from production code
|
Contributor
|
@stefanseifert IIRC you have a Windows system, can you please check this PR? I get constant failures in the CI for Windows machines, and I would like to understand if this is a Windows problem or rather a CI issue. |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
java.util.TimerinstartProcessing()with aScheduledExecutorServicethat is properly shut down ondeactivate(), preventing thread leakssetScheduler()so tests can inject a controllable schedulertestTopologyChangeto use aManualSchedulerthat captures tasks for on-demand execution, eliminating the flakyThread.sleep(4000)and making the test deterministic and ~8x fasterTest plan
mvn test -Dtest=JobManagerConfigurationTest#testTopologyChange— passes in ~1s (was ~9s)mvn test— all 109 tests pass🤖 Generated with Claude Code