Skip to content

Fix error reporting in Schedulers - #2036

Open
mikkolaj wants to merge 3 commits into
monix:mainfrom
AVSystem:fix-error-reporting-to-upstream
Open

Fix error reporting in Schedulers#2036
mikkolaj wants to merge 3 commits into
monix:mainfrom
AVSystem:fix-error-reporting-to-upstream

Conversation

@mikkolaj

@mikkolaj mikkolaj commented Apr 16, 2026

Copy link
Copy Markdown

Original PR: AVSystem#15

Summary

In JDK 25 ForkJoinPool implements ScheduledExecutorService. That change exposed a long-standing gap in Scheduler
error reporting - the failure of a scheduled task could be lost.

Problem

An uncaught failure of a task given to scheduleOnce, scheduleAtFixedRate or scheduleWithFixedDelay did not reach
the Scheduler's UncaughtExceptionReporter when the scheduler was backed by a bare ScheduledExecutorService:
the executor captures the failure in the Future it returns, and Monix used to discard that Future.

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 own afterExecute hook.
From JDK 25 Scheduler.computation and Scheduler.forkJoin are backed by a ForkJoinPool that 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 to setTimeout without the
reporting wrapper that execute uses.

Changes

  1. The scheduled methods wrap the task so its failure reaches the scheduler's UncaughtExceptionReporter. Failures that NonFatal does not match are reported and then re-thrown. This covers Scheduler.computation and Scheduler.forkJoin on JDK 25.
  2. The Scala.js AsyncScheduler.scheduleOnce gets the wrapping that its execute already uses, so a failure that NonFatal matches reaches the scheduler's UncaughtExceptionReporter instead of the ExecutionContext. Its periodic methods are built on scheduleOnce, so they report their failures too.
  3. Stopping after failure (the behaviour scheduleAtFixedRate already documented) is now documented on scheduleWithFixedDelay as well (matching their behavior and JDK docs).
  4. Reordered InterceptRunnable.apply so a null reporter short-circuits before the TrampolinedRunnable case, returning the runnable unwrapped instead of a wrapper that NullPointerExceptions on the first failure.

Not addressed here

  • On a Scheduler over a caller-supplied, plain ExecutorService, the fatal failures of scheduled tasks still reach the thread uncaught exception handler instead of the reporter.
  • On a Scheduler over a caller-supplied ScheduledExecutorService, a fatal failure of a task given to execute, or to scheduleOnce with a non-positive delay, is still invisible (Scheduler.computation and Scheduler.forkJoin on JDK 25 are unaffected).
  • A reporter installed with withUncaughtExceptionReporter is still ignored by the scheduled methods on
    • the wrappers returned from withUncaughtExceptionReporter and withExecutionModel
    • on Scheduler.singleThread and Scheduler.fixedPool
  • Scheduler.singleThread and Scheduler.fixedPool still ignore the reporter they are given when reportFailure is called from outside their own threads.
  • Fatal failures on ExecutionContext-backed schedulers still reach the ExecutionContext instead of the reporter.

Incidental cleanup

  • Dropped the unused deprecated auxiliary constructors of the private ExecutorScheduler subclasses FromSimpleExecutor and FromScheduledExecutor. A matching exclusion is added to MimaFilters.
  • ComputationSchedulerSuite builds its scheduler with Scheduler.computation rather than Scheduler.forkJoin, which ForkJoinSchedulerSuite already covers.

@alexandru

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

@mikkolaj
mikkolaj force-pushed the fix-error-reporting-to-upstream branch from 899ee7a to b17d816 Compare August 5, 2026 13:53

@mikkolaj mikkolaj left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Codex review was helpful. Additionally, I let Claude review it locally, which showed me a few other defects this PR had. Namely:

  • Restricting FromScheduledExecutor usage wasn't a good thing to do - doing the scheduling on the native pool has its benefits (e.g. sub-millisecond scheduleAtFixedRate periods and less stress on the shared, single-threaded Defaults.scheduledExecutor).
  • The Scala.js AsyncScheduler also failed to respect the configured UncaughtExceptionReporter.
  • Failures not matched by NonFatal could escape the configured UncaughtExceptionReporter.

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.

@mikkolaj
mikkolaj force-pushed the fix-error-reporting-to-upstream branch from b17d816 to cebc644 Compare August 7, 2026 15:55
Mikołaj Bul added 3 commits August 7, 2026 20:08
- 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
@mikkolaj
mikkolaj force-pushed the fix-error-reporting-to-upstream branch from cebc644 to d01ac80 Compare August 7, 2026 18:48
@mikkolaj

mikkolaj commented Aug 7, 2026

Copy link
Copy Markdown
Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create a Codex account and connect to github.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants