sharanggupta opened a new pull request, #4400:
URL: https://github.com/apache/logging-log4j2/pull/4400
Fixes #4399.
Instants before the epoch were mishandled in two places, because a
truncating `/` or `%` was used where a floor division is needed:
* `MutableInstant.initFromEpochMilli(long, int)` split a negative
`epochMilli` with `/ 1000`, so `-61_500 ms` became `epochSecond=-61,
nanoOfSecond=-500_000_000`. That breaks the `0 <= nanoOfSecond < 1_000_000_000`
invariant that `initFromEpochSecond` enforces and every consumer expects. It
now uses `Math.floorDiv`, which gives `epochSecond=-62,
nanoOfSecond=500_000_000`.
* `InstantPatternDynamicFormatter` keyed its per-minute cache with
`epochSecond / 60` and rendered the seconds with `epochSecond % 60`. Before the
epoch the seconds were negative and the wrong cached minute was selected:
`%d{HH:mm:ss.SSS}` printed `23:59:+'.000` for `-59_000 ms` instead of
`23:59:01.000`. These now use `Math.floorDiv` and `Math.floorMod`.
Both methods exist since Java 8, so there is no change to the supported
baseline. For instants after the epoch the results are identical, because floor
and truncating division agree for non-negative operands.
Deliberately not included: `InstantPatternThreadLocalCachedFormatter` starts
its cache with the epoch instant `-1`, so the first event on a thread at
exactly second (or millisecond) `-1` prints an empty string. That is described
under "Related" in #4399. I left it out because #4322 rewrites that class.
## Tests
* `MutableInstantTest.testInitFromEpochMilliNormalizesTheNanoOfSecond`:
seven `(epochMilli, nanoOfMilli)` cases, checking the normalized second and
nano and that `getEpochMillisecond()` and `getNanoOfMillisecond()` round-trip.
Four fail on `2.x`; the whole-second case and the two cases after the epoch
pass before and after.
*
`InstantPatternDynamicFormatterTest.output_before_the_epoch_should_match_DateTimeFormatter`:
six patterns (`s`, `ss`, `mm:ss`, `HH:mm:ss.SSS`, with micros, with an offset)
times eight instants before the epoch, compared with `DateTimeFormatter` as the
existing `output_should_match_DateTimeFormatter` does. 42 of the 48 cases fail
on `2.x`; the six that pass are the `-60 s` cases, which fall on a minute
boundary.
## Verification
* The `core.time`, `core.util.internal.instant`, `core.util.datetime`,
`core.pattern`, `core.layout` and `core.impl` test packages of
`log4j-core-test` pass in the default Surefire execution (8,675 tests, 0
failures), and spotless is green on all modules. The
`test-without-jackson-databind` execution of this module cannot load the
Jackson-based layout tests that my package filter selects, so I read the
results per execution.
* Outside the repository, I compared `InstantPatternDynamicFormatter` with
`DateTimeFormatter` over random patterns, zones, locales and instants from year
0 to 9999, about 710,000 comparisons over three seeds. After this change the
only differences are zones whose offset has a seconds part (for example
`Africa/Monrovia` before 1972, which `InstantPatternDynamicFormatterTest`
already excludes) and the `-1` case above.
* I did not run the complete `./mvnw verify` of the whole reactor locally
and rely on CI for it.
Generative-AI disclosure (ASF guidance): Claude Code (Anthropic) assisted in
finding the problem, drafting the change, tests and this description; the
result was reviewed, the new tests were confirmed to fail on `2.x` and pass
with the change, and the tests and checks above were run before opening this PR.
## Checklist
* [x] Base your changes on `2.x` branch if you are targeting Log4j 2; use
`main` otherwise
* [ ] `./mvnw verify` succeeds: not run for the whole reactor locally, see
Verification above
* [x] Non-trivial changes contain an entry file in the
`src/changelog/.2.x.x` directory
* [x] Tests are provided (`MutableInstantTest`: 1 new parameterized test,
`InstantPatternDynamicFormatterTest`: 1 new parameterized test)
--
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]
