LOG4J2-2073: Fix NPE in ConfigurationScheduler when scheduling cron tasks#3941
LOG4J2-2073: Fix NPE in ConfigurationScheduler when scheduling cron tasks#3941bhawnapannu2701 wants to merge 1 commit intoapache:2.xfrom
Conversation
…add null-guards and test
vy
left a comment
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Would you implement the following changes, please?
- This test is not effective. That is, it even passes without the fix. Could you make sure this test fails without the fix?
- Could you move this test from
log4j-core/src/testtolog4j-core-test/src/test? - Could you add the license header?
- Could you make sure
./mvnw verify -pl :log4j-core,:log4j-core-testpasses before submitting your changes?
There was a problem hiding this comment.
- Remove the
FIX (part 1)-sort comments, please. Instead, if something is not obvious at first glance, explain in the rationale, when necessary. - Run linter:
./mvnw spotless:apply -pl :log4j-core - Don't touch/change unrelated lines
|
Hi @vy, I noticed that this PR is still open and has some review comments pending. If this is still active, I’d be happy to help move it forward and submit an update. Thanks for your time and for maintaining the project. |
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.