Skip to content

Conversation

bhawnapannu2701
Copy link

Problem
ConfigurationScheduler.scheduleWithCron created a CronRunnable, scheduled it, and only afterwards assigned its CronScheduledFuture.
Under fast-firing cron expressions the runnable could execute before its future was set, causing a NullPointerException when calling scheduledFuture.getFireTime().

Root cause
Race condition between scheduling the task and assigning the CronScheduledFuture to the runnable.

Fix

Assign a placeholder CronScheduledFuture to the runnable before scheduling.

Update that placeholder with the real ScheduledFuture immediately after scheduling.

Add null-guards in CronRunnable.run() and handle first execution safely.

Enhance toString() to tolerate an unassigned future.

Tests
Added CronSchedulerNpeTest which schedules a cron expression firing every second and asserts that it runs without throwing.
Before the fix it reproduced the NPE; after the fix all tests pass.

Risks
Minimal. Changes are contained to cron scheduling logic and do not alter normal scheduling semantics.

@vy vy self-assigned this Oct 3, 2025
@vy vy added bug Incorrect, unexpected, or unintended behavior of existing code configuration Affects the configuration system in a general way labels Oct 3, 2025
@vy vy added this to the 2.26.0 milestone Oct 3, 2025
Copy link
Member

@vy vy left a comment

Choose a reason for hiding this comment

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

@bhawnapannu2701, thanks so much for the PR. Please see the remarks I've shared.

I strongly advice you to read the Development guide. This will make life easier for both of us.

Copy link
Member

Choose a reason for hiding this comment

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

Would you implement the following changes, please?

  1. This test is not effective. That is, it even passes without the fix. Could you make sure this test fails without the fix?
  2. Could you move this test from log4j-core/src/test to log4j-core-test/src/test?
  3. Could you add the license header?
  4. Could you make sure ./mvnw verify -pl :log4j-core,:log4j-core-test passes before submitting your changes?

Copy link
Member

Choose a reason for hiding this comment

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

  1. Remove the FIX (part 1)-sort comments, please. Instead, if something is not obvious at first glance, explain in the rationale, when necessary.
  2. Run linter: ./mvnw spotless:apply -pl :log4j-core
  3. Don't touch/change unrelated lines

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect, unexpected, or unintended behavior of existing code configuration Affects the configuration system in a general way
Projects
Status: To triage
Development

Successfully merging this pull request may close these issues.

2 participants