Fix error reporting in Schedulers - #2036
Conversation
|
@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".
899ee7a to
b17d816
Compare
mikkolaj
left a comment
There was a problem hiding this comment.
The Codex review was helpful. Additionally, I let Claude review it locally, which showed me a few other defects this PR had. Namely:
- Restricting
FromScheduledExecutorusage wasn't a good thing to do - doing the scheduling on the native pool has its benefits (e.g. sub-millisecondscheduleAtFixedRateperiods and less stress on the shared, single-threadedDefaults.scheduledExecutor). - The Scala.js
AsyncScheduleralso failed to respect the configuredUncaughtExceptionReporter. - Failures not matched by
NonFatalcould escape the configuredUncaughtExceptionReporter.
This led me to a different approach, described in more detail in the PR description. I also rebased and reorganized the commits, so that they're hopefully easier to follow.
b17d816 to
cebc644
Compare
- Merged `ScheduledExecutorToSchedulerSuite` into `ExecutorSchedulerSuite` to remove duplication. - Extend `ExecutorSchedulerSuite` with the scheduled error-reporting and stop-after-failure cases - Build `ComputationSchedulerSuite`'s scheduler with `Scheduler.computation` - Extend `UncaughtExceptionReporterBaseSuite` with the `scheduleX` cases and close the `SchedulerService`-backed schedulers it builds - Extend `AsyncSchedulerJSSuite` with the stop-after-failure cases
…acked Schedulers - Fix reporting in the `scheduleX` methods of `ExecutorScheduler.FromScheduledExecutor`, where a task failure was captured in a discarded future and lost - Stopping a periodic task after a failure is now cancellation-based - Fix constructing `InterceptRunnable` from a `TrampolinedRunnable` and a `null` reporter - Drop the unused deprecated constructors of the private ExecutorScheduler subclasses
- Fix reporting in the `scheduleX` methods of the Scala.js `AsyncScheduler` - Drop the `null` reporter guard in `executeAsync`, which `InterceptRunnable` handles
cebc644 to
d01ac80
Compare
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
Original PR: AVSystem#15
Summary
In JDK 25
ForkJoinPoolimplementsScheduledExecutorService. That change exposed a long-standing gap inSchedulererror reporting - the failure of a scheduled task could be lost.
Problem
An uncaught failure of a task given to
scheduleOnce,scheduleAtFixedRateorscheduleWithFixedDelaydid not reachthe
Scheduler'sUncaughtExceptionReporterwhen the scheduler was backed by a bareScheduledExecutorService:the executor captures the failure in the
Futureit returns, and Monix used to discard thatFuture.Before JDK 25 this stayed mostly invisible, because Monix's own builders either avoided that path or supplied
an
AdaptedThreadPoolExecutor, which reports such failures through its ownafterExecutehook.From JDK 25
Scheduler.computationandScheduler.forkJoinare backed by aForkJoinPoolthat takes that path,so they lost the failures of their scheduled tasks too.
On Scala.js the analogous gap was in
AsyncScheduler.scheduleOnce, which handed the task tosetTimeoutwithout thereporting wrapper that
executeuses.Changes
UncaughtExceptionReporter. Failures thatNonFataldoes not match are reported and then re-thrown. This coversScheduler.computationandScheduler.forkJoinon JDK 25.AsyncScheduler.scheduleOncegets the wrapping that itsexecutealready uses, so a failure thatNonFatalmatches reaches the scheduler'sUncaughtExceptionReporterinstead of theExecutionContext. Its periodic methods are built onscheduleOnce, so they report their failures too.scheduleAtFixedRatealready documented) is now documented onscheduleWithFixedDelayas well (matching their behavior and JDK docs).InterceptRunnable.applyso anullreporter short-circuits before theTrampolinedRunnablecase, returning the runnable unwrapped instead of a wrapper thatNullPointerExceptions on the first failure.Not addressed here
Schedulerover a caller-supplied, plainExecutorService, the fatal failures of scheduled tasks still reach the thread uncaught exception handler instead of the reporter.Schedulerover a caller-suppliedScheduledExecutorService, a fatal failure of a task given toexecute, or toscheduleOncewith a non-positive delay, is still invisible (Scheduler.computationandScheduler.forkJoinon JDK 25 are unaffected).withUncaughtExceptionReporteris still ignored by the scheduled methods onwithUncaughtExceptionReporterandwithExecutionModelScheduler.singleThreadandScheduler.fixedPoolScheduler.singleThreadandScheduler.fixedPoolstill ignore the reporter they are given whenreportFailureis called from outside their own threads.ExecutionContext-backed schedulers still reach theExecutionContextinstead of the reporter.Incidental cleanup
ExecutorSchedulersubclassesFromSimpleExecutorandFromScheduledExecutor. A matching exclusion is added toMimaFilters.ComputationSchedulerSuitebuilds its scheduler withScheduler.computationrather thanScheduler.forkJoin, whichForkJoinSchedulerSuitealready covers.