Skip to content

Fix the overflow bound direction in Instants.toMillisSince(Instant) - #1766

Open
renechoi wants to merge 1 commit into
apache:masterfrom
renechoi:fix-instants-tomillissince-overflow-bound
Open

Fix the overflow bound direction in Instants.toMillisSince(Instant)#1766
renechoi wants to merge 1 commit into
apache:masterfrom
renechoi:fix-instants-tomillissince-overflow-bound

Conversation

@renechoi

@renechoi renechoi commented Aug 2, 2026

Copy link
Copy Markdown

Instants.toMillisSince(Instant) documents two bounds:

  • If the duration milliseconds are greater than Long.MAX_VALUE, then return Long.MAX_VALUE.
  • If the duration milliseconds are lesser than Long.MIN_VALUE, then return Long.MIN_VALUE.

It selects the bound with toBound(instant2, Long.MIN_VALUE, Long.MAX_VALUE), which tests the sign of the instant's epoch second. The value being bound is DurationUtils.since(instant), the duration from that instant to now, and its sign is the opposite one: a past instant yields a positive duration, a future instant a negative one. The suite already pins that convention in testToMillisSincePastInstantIsPositive and testToMillisSinceFutureInstantIsNegative.

Duration.toMillis() only overflows past roughly ±292 million years, so an instant is either far enough in the past that both the duration and the epoch second are extreme, or far enough in the future that they are, and the two always disagree in sign. Every reachable overflow is therefore bound to the wrong end. Against master (8f8f3b2):

Duration.between(Instant.MIN, now).isNegative() = false   (seconds=31557015952864320)
  Javadoc says: millis > Long.MAX_VALUE  ->  9223372036854775807
  actual                                ->  -9223372036854775808

Duration.between(Instant.MAX, now).isNegative() = true    (seconds=-31556888078758080)
  Javadoc says: millis < Long.MIN_VALUE  ->  -9223372036854775808
  actual                                ->   9223372036854775807

toEpochMillis is unaffected: there the quantity being bound is the instant, so the epoch-second test in toBound is the right discriminator. Only toMillisSince reuses it against a quantity of the opposite sign.

The fix keeps the Duration and binds on duration.isNegative(), which is the quantity the Javadoc describes. Results that do not overflow are unchanged.

The two existing overflow tests asserted the old behavior, and their Javadoc says so explicitly ("the bound is Long.MAX_VALUE because the instant's epoch second is positive"), so they are inverted and renamed for the direction they now cover. Both fail without this change. Instants is new in 3.21.0 and not yet released, so no released behavior changes.

Verified locally on JDK 21 with the default goal (mvn): 89186 tests, 0 failures, 0 errors, and rat, checkstyle, japicmp, spotbugs, pmd and javadoc all clean.

The Javadoc states that a result greater than Long.MAX_VALUE is bound to
Long.MAX_VALUE and one lesser than Long.MIN_VALUE to Long.MIN_VALUE, but the
bound was selected from the sign of the instant's epoch second. The value being
bound is the duration from that instant to now, whose sign is the opposite, so
both reachable overflow cases returned the wrong end: Instant.MIN gave
Long.MIN_VALUE where the contract asks for Long.MAX_VALUE, and Instant.MAX the
reverse.

Bind on the sign of the duration instead. The two tests that pinned the previous
behavior are updated; both fail without this change.
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.

1 participant