8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant #18674

jaikiran · 2024-04-08T09:52:39Z

Can I please get a review of this change which proposes to fix the issue noted in https://bugs.openjdk.org/browse/JDK-8212895?

As noted in that issue, the ChronoField.INSTANT_SECONDS currently is initialized to have a minimum and maximum values of Long.MIN_VALUE and LONG.MAX_VALUE respectively. However, java.time.Instant only supports -31557014167219200L and 31556889864403199L as minimum and maximum values for the epoch second.

The commit in this PR updates the ChronoField.INSTANT_SECONDS's value range to match the supported min and max values of Instant (as suggested by Stephen in that JBS issue). This commit also introduces a test to verify this change. This new test method as well as existing tests in tier1, tier2 and tier3 continue to pass with this change.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8330135 to be approved

Issues

JDK-8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant (Bug - P4)
JDK-8330135: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant (CSR)

Reviewers

Roger Riggs (@RogerRiggs - Reviewer)
Naoto Sato (@naotoj - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18674/head:pull/18674
$ git checkout pull/18674

Update a local copy of the PR:
$ git checkout pull/18674
$ git pull https://git.openjdk.org/jdk.git pull/18674/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18674

View PR using the GUI difftool:
$ git pr show -t 18674

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18674.diff

Webrev

Link to Webrev Comment

…of Instant

bridgekeeper · 2024-04-08T09:53:38Z

👋 Welcome back jpai! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-04-08T09:54:47Z

@jaikiran This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant

Reviewed-by: rriggs, naoto

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 131 new commits pushed to the master branch:

76cbe48: 8329430: MetaspaceShared::preload_and_dump should clear pending exception
f7c8413: 8326116: JFR: Add help option to -XX:StartFlightRecording
941bee1: 8327640: Allow NumberFormat strict parsing
2ede143: 8330279: Typo in java.text.Bidi class description
90df3b7: 8329190: (ch) DatagramChannel.receive should throw ClosedChannelException when called on closed channel
f11a496: 8180450: secondary_super_cache does not scale well
bfff02e: 8330165: C2: make superword consistently use PhaseIdealLoop::register_new_node()
e073d5b: 8329665: fatal error: memory leak: allocating without ResourceMark
6e77d91: 8330261: Clean up linking steps
61fa4d4: 8330351: Remove the SHARED_LIBRARY and STATIC_LIBRARY macros
... and 121 more: https://git.openjdk.org/jdk/compare/d771ec6296bd3bb17ac2dd79671fdb6f154b4ea5...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-04-08T09:55:10Z

@jaikiran The following labels will be automatically applied to this pull request:

core-libs
i18n

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-04-08T09:58:16Z

Webrevs

jaikiran · 2024-04-08T11:19:13Z

Given that this is a observable change through a public API, I think a csr will be needed for this. I'll draft one shortly.

jaikiran · 2024-04-08T11:19:17Z

/csr needed

openjdk · 2024-04-08T11:19:30Z

@jaikiran has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@jaikiran please create a CSR request for issue JDK-8212895 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

naotoj · 2024-04-08T18:00:02Z

src/java.base/share/classes/java/time/temporal/ChronoField.java

@@ -587,7 +587,8 @@ public enum ChronoField implements TemporalField {
     * This field is strictly defined to have the same meaning in all calendar systems.
     * This is necessary to ensure interoperation between calendars.
     */
-    INSTANT_SECONDS("InstantSeconds", SECONDS, FOREVER, ValueRange.of(Long.MIN_VALUE, Long.MAX_VALUE)),
+    // ValueRange matches the min and max epoch second supported by java.time.Instant


Would we want to include this in the javadoc?

The code should reference the constants in Instant.java (though may need to make them package private.)

The javadoc can/should reference Instant.MIN and Instant.MAX (as the test does).

Hello Roger,

The code should reference the constants in Instant.java (though may need to make them package private.)

The ChronoField class and the Instant class reside in separate packages, so making these two fields in Instant, package private will not help. I will have to make them public, which I think probably isn't a good idea. Unless you think we should do it? There is one other place already in the JDK where we have copy/pasted these values src/java.base/share/classes/java/nio/file/attribute/FileTime.java, so maybe we can continue with this copy/paste here too?

As for the javadoc, after we decide about this field access detail, I'll update it accordingly to note that the values min and max range complies with the min and max epoch seconds supported by Instant.

Should INSTANT_SECONDS("InstantSeconds", SECONDS, FOREVER, ValueRange.of(Instant.MIN.getEpochSecond(), Instant.MAX.getEpochSecond())), work?

Hello Naoto, that's a very good point. I was too caught up with the constant values that it didn't occur to me that the Instant.MIN and Instant.MAX public fields could be used for this. I have followed your suggestion and updated the PR. I have also updated the javadoc to link to Instant.MIN and Instant.MAX as the supported epoch second range.

The test continues to pass with this change and fails (as expected) without the source change.

Good, that was going to be my backup suggestion; trying to avoid a method call even for the init. :)

…oded values

naotoj

Thanks, Jai. The code change looks good. (Left a minor nit)

naotoj · 2024-04-09T16:29:07Z

src/java.base/share/classes/java/time/temporal/ChronoField.java

@@ -586,8 +586,11 @@ public enum ChronoField implements TemporalField {
     * <p>
     * This field is strictly defined to have the same meaning in all calendar systems.
     * This is necessary to ensure interoperation between calendars.
+     * <p>
+     * Range of {@code InstantSeconds} is between {@link Instant#MIN} and {@link Instant#MAX}


Nit: InstantSeconds -> INSTANT_SECONDS

Hello Naoto, I had used InstantSeconds to keep it consistent with how a similar doc is used for the EPOCH_DAY field. Let me know if you still prefer this to be INSTANT_SECONDS and I will update it.

With the @code tag, I initially thought it can be used programmatically, but apparently, it was simply an Enum name. Users may not notice it till they see the source (or run its toString()). I am fine with either way, but probably keep consistent with EPOCH_DAY where it is not using @code tag.

Hello Naoto, that's good point. I've updated the PR to remove {@code}.

jaikiran · 2024-04-10T16:14:45Z

Now that the changes have been finalized, I'll raise the CSR tomorrow.

naotoj

Looks good, Jai. Thanks for the fix!

jaikiran · 2024-04-12T04:04:14Z

I've now created the CSR https://bugs.openjdk.org/browse/JDK-8330135. I'll move it to Finalized once I get a review for it.

jaikiran · 2024-04-16T06:08:00Z

The CSR for this has been approved. I'll integrate this PR tomorrow (around 24 hours from now).

Naoto, Roger, should we consider a release note for this change or is the CSR itself enough?

naotoj · 2024-04-16T16:48:34Z

Naoto, Roger, should we consider a release note for this change or is the CSR itself enough?

I think CSR is enough, as users' chance of encountering any issue is very slim. My $0.02

RogerRiggs · 2024-04-16T19:18:14Z

The CSR is enough, no-one else except the filer will even notice.

jaikiran · 2024-04-17T10:57:43Z

Thank you Naoto and Roger for the inputs and the reviews.

jaikiran · 2024-04-17T10:57:54Z

/integrate

openjdk · 2024-04-17T10:59:13Z

Going to push as commit 89129e3.
Since your change was applied there have been 149 commits pushed to the master branch:

9445047: 8330163: C2: improve CMoveNode::Value() when condition is always true or false
d2f9a1e: Merge
33d7127: 8322122: Enhance generation of addresses
8de2f6f: 8319851: Improve exception logging
0203c7e: 8318340: Improve RSA key implementations
2885469: 8315708: Enhance HTTP/2 client usage
e4021ad: 8330272: Wrong javadoc for ValueLayout.JAVA_LONG/JAVA_DOUBLE on x86 32bit
3ccbc6d: 8330280: SharedRuntime::get_resolved_entry should not return c2i entry if the callee is special native intrinsic
7744b00: 8327946: containers/docker/TestJFREvents.java fails when host kernel config vm.swappiness=0 after JDK-8325139
9d63fee: 8330262: C2: simplify transfer of GC barrier data from Ideal to Mach nodes
... and 139 more: https://git.openjdk.org/jdk/compare/d771ec6296bd3bb17ac2dd79671fdb6f154b4ea5...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-04-17T10:59:19Z

@jaikiran Pushed as commit 89129e3.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range …

1fea0f7

…of Instant

openjdk bot added the rfr Pull request is ready for review label Apr 8, 2024

openjdk bot added core-libs core-libs-dev@openjdk.org i18n i18n-dev@openjdk.org labels Apr 8, 2024

openjdk bot added the csr Pull request needs approved CSR before integration label Apr 8, 2024

naotoj reviewed Apr 8, 2024

View reviewed changes

Naoto's suggestion - use Instant.MIN and Instant.MAX instead of hardc…

6e53577

…oded values

RogerRiggs approved these changes Apr 9, 2024

View reviewed changes

naotoj reviewed Apr 9, 2024

View reviewed changes

no need for {@code} in javadoc

ca26568

naotoj approved these changes Apr 10, 2024

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Apr 15, 2024

RogerRiggs approved these changes Apr 16, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Apr 17, 2024

openjdk bot closed this Apr 17, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 17, 2024

jaikiran deleted the 8212895 branch April 17, 2024 11:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant #18674

8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant #18674

jaikiran commented Apr 8, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Apr 8, 2024

openjdk bot commented Apr 8, 2024 •

edited

Loading

openjdk bot commented Apr 8, 2024

mlbridge bot commented Apr 8, 2024 •

edited

Loading

jaikiran commented Apr 8, 2024

jaikiran commented Apr 8, 2024

openjdk bot commented Apr 8, 2024

naotoj Apr 8, 2024

RogerRiggs Apr 8, 2024 •

edited

Loading

jaikiran Apr 9, 2024

naotoj Apr 9, 2024

jaikiran Apr 9, 2024

RogerRiggs Apr 9, 2024

naotoj left a comment

naotoj Apr 9, 2024

jaikiran Apr 10, 2024

naotoj Apr 10, 2024

jaikiran Apr 10, 2024

jaikiran commented Apr 10, 2024

naotoj left a comment

jaikiran commented Apr 12, 2024

jaikiran commented Apr 16, 2024

naotoj commented Apr 16, 2024

RogerRiggs commented Apr 16, 2024

jaikiran commented Apr 17, 2024

jaikiran commented Apr 17, 2024

openjdk bot commented Apr 17, 2024

openjdk bot commented Apr 17, 2024

8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant #18674

8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant #18674

Conversation

jaikiran commented Apr 8, 2024 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Apr 8, 2024

openjdk bot commented Apr 8, 2024 • edited Loading

openjdk bot commented Apr 8, 2024

mlbridge bot commented Apr 8, 2024 • edited Loading

Webrevs

jaikiran commented Apr 8, 2024

jaikiran commented Apr 8, 2024

openjdk bot commented Apr 8, 2024

Choose a reason for hiding this comment

RogerRiggs Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naotoj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaikiran commented Apr 10, 2024

naotoj left a comment

Choose a reason for hiding this comment

jaikiran commented Apr 12, 2024

jaikiran commented Apr 16, 2024

naotoj commented Apr 16, 2024

RogerRiggs commented Apr 16, 2024

jaikiran commented Apr 17, 2024

jaikiran commented Apr 17, 2024

openjdk bot commented Apr 17, 2024

openjdk bot commented Apr 17, 2024

jaikiran commented Apr 8, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Apr 8, 2024 •

edited

Loading

mlbridge bot commented Apr 8, 2024 •

edited

Loading

RogerRiggs Apr 8, 2024 •

edited

Loading