Fix error reporting in Schedulers#2036
Conversation
- fix updating UncaughtExceptionReporter for Scheduler backed by ScheduledThreadPoolExecutor - fix updating UncaughtExceptionReporter for WrappedSchedulers - fix error reporting for wrapped schedulers
…ceptionReporter override is present
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 899ee7aa62
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| s.scheduleWithFixedDelay(initialDelay, delay, unit, InterceptRunnable(r, reporter)) | ||
| override def scheduleAtFixedRate(initialDelay: Long, period: Long, unit: TimeUnit, r: Runnable): Cancelable = | ||
| s.scheduleAtFixedRate(initialDelay, period, unit, r) | ||
| s.scheduleAtFixedRate(initialDelay, period, unit, InterceptRunnable(r, reporter)) |
There was a problem hiding this comment.
Preserve periodic task suppression after failures
When withUncaughtExceptionReporter is used on a scheduler whose periodic methods come from ReferenceScheduler (for example schedulers backed by a plain ExecutorService), wrapping the user runnable here catches and swallows the exception before the underlying ReferenceScheduler.scheduleAtFixedRate loop sees it. That loop only stops because r.run() throws, and the public Scheduler docs say later fixed-rate executions are suppressed after an exception; with this wrapper the task keeps rescheduling forever after reporting the failure.
Useful? React with 👍 / 👎.
| def withUncaughtExceptionReporter(reporter: UncaughtExceptionReporter): AdaptedScheduledThreadPoolExecutor = | ||
| new AdaptedScheduledThreadPoolExecutor(corePoolSize, factory, reporter) |
There was a problem hiding this comment.
Reuse the existing executor when swapping reporters
For singleThread/fixedPool schedulers, calling withUncaughtExceptionReporter now constructs an entirely new ScheduledThreadPoolExecutor instead of returning a view over the same service. In the common pattern of creating a scheduler and immediately overriding its reporter, any already-started original pool is left independent and won't be shut down when the returned scheduler is shut down; existing scheduled work also remains attached to the old reporter/executor, unlike the other withUncaughtExceptionReporter implementations that keep the same executor.
Useful? React with 👍 / 👎.
Original PR: AVSystem#15