tupelo-schneck commented on code in PR #4226:
URL: https://github.com/apache/logging-log4j2/pull/4226#discussion_r3853936165
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/CronExpression.java:
##########
@@ -1581,6 +1581,13 @@ protected Date getTimeBefore(final Date targetDate) {
Date prevFireTime;
do {
final Date prevCheckDate = new Date(start.getTime() -
minIncrement);
+ // `getTimeAfter()` never returns a fire time before 1970, so once
the candidate
+ // date precedes `MIN_DATE` the loop condition below can no longer
be satisfied.
+ // Bail out here, otherwise the search walks back millennia one
increment at a
+ // time before `getTimeAfter()` finally gives up at the calendar's
upper bound.
+ if (prevCheckDate.before(MIN_DATE)) {
+ return null;
+ }
Review Comment:
Applied, thank you — this is a real defect in what I wrote, not a style
preference.
I reproduced the nondeterminism: two JVMs started seconds apart print
`MIN_DATE` as `09:40:09` and `09:40:17`, since `MIN_CAL.set(1970, 0, 1)` leaves
the time fields alone. Your example reproduces exactly — for `0 0 0 * * ?` at
`1970-01-02 06:00`, `2.x` returns `1970-01-02 00:00` where the `MIN_DATE` bound
returned `null`. A correct answer silently replaced, and which answer you got
depended on when the process started.
I also tried bounding on the local 1970 instant in the expression's own
zone, on the theory that `getTime() < 0` is a UTC instant test while
`getTimeAfter()` floors at local 1970. Across `America/New_York`, `UTC` and
`Australia/Sydney` it produced results identical to the epoch bound on every
case I tried, so the simpler form is the right one.
Added `testPrevFireTimeJustAfterEpochIsUnaffectedByBound` to pin this — it
fails with the `MIN_DATE` bound and passes with the epoch, in a fixed zone so
it does not depend on the CI host.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
