Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8266846: Add java.time.InstantSource #4016

Closed
wants to merge 7 commits into from

Conversation

jodastephen
Copy link
Contributor

@jodastephen jodastephen commented May 13, 2021


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4016/head:pull/4016
$ git checkout pull/4016

Update a local copy of the PR:
$ git checkout pull/4016
$ git pull https://git.openjdk.java.net/jdk pull/4016/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4016

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4016.diff

@bridgekeeper
Copy link

bridgekeeper bot commented May 13, 2021

👋 Welcome back scolebourne! 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
Copy link

openjdk bot commented May 13, 2021

@jodastephen The following label will be automatically applied to this pull request:

  • core-libs

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

@openjdk openjdk bot added the core-libs core-libs-dev@openjdk.org label May 13, 2021
*
* @implSpec
* This interface must be implemented with care to ensure other classes operate correctly.
* All implementations that can be instantiated must be final, immutable and thread-safe.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I'm not an openjdk reviewer)

I'm wondering if we can revisit this "immutable" requirement as it is being shuffled from Clock to InstantSource.

This precludes an implementation that might be useful for testing, where the time provided by a single InstantSource instance can be adjusted manually in the test code. To me that is a notable downside (worth breaking the contract for), and it's not so clear what the upside is. "Immutable" seems like an odd way to describe the system clock anyway.

Do you know if the people who already use their own InstantSource / TimeSource / Supplier<Instant> have a mutable version that they use in tests? I would be surprised if they're all satisfied with InstantSource.fixed, but perhaps my intuition is wrong here.

Copy link
Contributor

@anthonyvdotbe anthonyvdotbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A nice addition to the JDK, thanks for taking this on.

Edit: this review is fully addressed (I can't find a way to mark it as resolved or minimize it somehow?)

* An instant source that always returns the latest time from
* {@link System#currentTimeMillis()} or equivalent.
*/
static final class SystemInstantSource implements InstantSource, Serializable {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to declare this as an enum? As doing so would simplify its implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My feeling is that in this case it is better to keep control of the kind of class being used.

* Related to this class is {@link InstantSource} which is an interface that only
* provides access to the current instant, and does not provide access to the time-zone.
* Where an application only requires the current instant {@code InstantSource} should
* be preferred to this class. For example, his might be the case where the time-zone
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

his -> this

* }
* }
* </pre>
* This approach allows an alternate source, such as {@link #fixed(Instant) fixed}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternate -> alternative? To me (being a non-native speaker) the latter reads more naturally

* This approach allows an alternate source, such as {@link #fixed(Instant) fixed}
* or {@link #offset(InstantSource, Duration) offset} to be used during testing.
* <p>
* The {@code system} factory method provide a source based on the best available
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

provide -> provides

* or {@link #offset(InstantSource, Duration) offset} to be used during testing.
* <p>
* The {@code system} factory method provide a source based on the best available
* system clock This may use {@link System#currentTimeMillis()}, or a higher
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing dot after "clock"

//-------------------------------------------------------------------------
/**
* Obtains a source that returns instants from the specified source with the
* specified duration added
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing dot to end the sentence

* provides access to the current instant, and does not provide access to the time-zone.
* Where an application only requires the current instant {@code InstantSource} should
* be preferred to this class. For example, his might be the case where the time-zone
* is provided by a separate user localization subsystem.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@see InstantSource would be preferred here.

Comment on lines 27 to 32
import java.time.Clock.FixedClock;
import java.time.Clock.OffsetClock;
import java.time.Clock.SourceClock;
import java.time.Clock.SystemClock;
import java.time.Clock.SystemInstantSource;
import java.time.Clock.TickClock;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of them are not used.

*
* @return a source that uses the best available system clock, not null
*/
public static InstantSource system() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public is redundant here, same for other methods.

@@ -1,5 +1,5 @@
/*
* Copyright (c) 2012, 2019, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2012, 2019, 2021, Oracle and/or its affiliates. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2019, should be removed.

@openjdk openjdk bot added the rfr Pull request is ready for review label May 15, 2021
@mlbridge
Copy link

mlbridge bot commented May 15, 2021

Webrevs

*
* @since 17
*/
public interface InstantSource {
Copy link

@ineuwirth ineuwirth May 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not we add @FunctionalInterface? I can easily imagine this interface being used in tests where we can define the InstantSource with lambdas.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FunctionalInterface isn't required for use by lambdas. I wasn't initially convinced using it here was the right choice.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, it's about signaling that it's safe to be used with a lambda/method-reference, so it's not required but a nice to have, very like @OverRide.

@rgoers
Copy link

rgoers commented May 15, 2021

Can you possibly find a way that this can be used in a garbage free manner? Providing a MutableInstant interface that allows the Instant object to be provided and have its values set by a currentInstant(MutableInstant instant) method would solve the problem nicely for Log4j - or any other app that is trying to avoid allocating new objects. I believe this also aligns with what Michael Hixson is asking for.

An alternative would be for java.time to maintain a pool of Instants that apps like Log4j can somehow control, but that would seem far more complicated.

@jodastephen
Copy link
Contributor Author

@rgoers Michael's request has been handled, which was a simple change to the wording. What you need cannot be accomplished because Instant has a restricted design due to the Valhalla project.

A MutableInstant class can have its own now() method that does whatever is necessary to initialize it, so there is no need to refer to it here.

@jodastephen
Copy link
Contributor Author

It would good to get confirmation in the review from @dholmes-ora or Mark Kralj-Taylor that my changes to the precise system clock are correct. Specifically, I moved the code from instance to static. I don't believe this changes the thread-safety, but it does need double checking (there was no discussion in the original thread concerning instance vs static).

Original thread: http://mail.openjdk.java.net/pipermail/core-libs-dev/2020-May/066670.html

@RogerRiggs
Copy link
Contributor

RogerRiggs commented May 17, 2021

/csr needed # The csr is already in process.

@openjdk
Copy link

openjdk bot commented May 17, 2021

@RogerRiggs usage: /csr [needed|unneeded], requires that the issue the pull request refers to links to an approved CSR request.

* <p>
* Instances of this interface are used to find the current instant.
* As such, an {@code InstantSource} can be used instead of {@link System#currentTimeMillis()}.
* <p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The word 'current' is likely to misleading here. The specification of an interface does not have any context in which to describe what the instant represents or what it is relative to.
Given the intended use cases, it is definitely not always related to System.currentTimeMillis()
which is bound to the running system.
i think the best you could say is that it returns an instant provided by the source as does the 'instance()' method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the definition used by Clock since Java 8. It is also the purpose of the interface. ie. this isn't an interface for providing instants in general, but for providing the current instant. I can clarify wrt the meaning of "current", but I don't see how that word can be avoided.

* @return a source that always returns the same instant, not null
*/
static InstantSource fixed(Instant fixedInstant) {
return Clock.fixed(fixedInstant, ZoneOffset.UTC);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instant might also implement InstantSource, returning itself.
This fixed method would be unnecessary because an Instant is itself a InstantSource.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I can see the appeal, I don't think this would be a good choice. Doing so would add three methods to Instant that are of no value. millis() would duplicate toEpochMilli(), withZone(zone) would duplicate atZone(zone) and instant() would return this. Moreover it doesn't respect the rationale of the interface, which is explictly to return the current instant, not any random instant.

Effectively, this proposal is a variation of Scala's implicit conversions which were rejected during the development of JSR-310 as being more likely to cause bugs than help developers,

*
* @implSpec
* This interface must be implemented with care to ensure other classes operate correctly.
* All implementations must be thread-safe.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be useful to expand on the invariants that must be maintained, or examples of incorrect implementations.

* <p>
* Implementations should implement {@code Serializable} wherever possible and must
* document whether or not they do support serialization.
*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ImplSpec needs to say how it is implemented.
The 'implements InstantSource' can not mandate any particular implementation. Its just an interface the real behavior comes from its implementations. In this case Clock. Referring to the static methods of InstantSource behavior may be sufficient because that behavior is concrete.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are plenty of examples of interfaces in java.time and elsewhere that apply restrictions to implementations. Nevertheless, for simplicity and expediency I have reinstated the implSpec on Clock

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, thanks.
Assertions in interfaces are toothless. Tests can only be written for implementations.

@RogerRiggs
Copy link
Contributor

/csr

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label May 17, 2021
@openjdk
Copy link

openjdk bot commented May 17, 2021

@RogerRiggs this pull request will not be integrated until the CSR request JDK-8266847 for issue JDK-8266846 has been approved.

@rgoers
Copy link

rgoers commented May 17, 2021

@jodastephen While an implementation can certainly implement the now() method there is nothing useful it can do in the JVM to get anything more than millisecond precision. It needs to perform the logic in currentInstant() which requires a call to VM.getNanoTimeAdjustment(localOffset) which it doesn't have access to. This is precisely why it needs to call Clock to pass in a structure that can be populated with the millis and nanos.

@jodastephen
Copy link
Contributor Author

FWIW, if there is a need to access VM.getNanoTimeAdjustment(localOffset) then that should be a separate issue. I don't personally think it would be approved given Valhalla is coming.

// bring back the offset in range. We use -1024 to make
// it more unlikely to hit the 1ns in the future condition.
localOffset = System.currentTimeMillis()/1000 - 1024;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible that after a fresh localOffset is retrieved, the thread is preempted and when it is scheduled again after a pause, the getNanoTimeAdjustment below returns -1 ? Would it help if instead of throwing exception, there was an infinite retry loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't my logic - it is existing code that has been moved. I'm not a fan of infinite retry loops as the can hang the system. But I'm happy to change it to work that way if there is a consensus to do so.

Copy link
Contributor

@plevart plevart May 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the localOffset passed to VM.getNanoTimeAdjustment() has to be in the range of current time +/- 2^32 seconds. This means the thread would have to sleep for ~136 years before the exception is triggered. I think this is more than enough. :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly. You could hit the 1ns if the operating system clock was concurrently moved backward by a NTP adjustment of ~ 1024 seconds at the same time that the offset is taken but this would be extremely unlikely - I don't believe it deserves any infinite retry loop. That was discussed at the time the enhancement that provides sub-millisecond precision was proposed, and it was rejected in favor of a one time retry.

var testInstantMillis = test.instant().toEpochMilli();
assertTrue(Math.abs(testMillis - millis) < 1000);
assertTrue(Math.abs(testInstantMillis - millis) < 1000);
assertSame(test.equals(InstantSource.system());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

')' is missing. Couple other locations.

@@ -272,7 +272,7 @@
* @return the current instant using the system clock, not null
*/
public static Instant now() {
return Clock.systemUTC().instant();
return Clock.currentInstant();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copyright year should change to 2021 for this change.

Comment on lines 85 to 86
assertSame(test.equals(InstantSource..fixed(instant));
assertEquals(test.hashCode(), InstantSource..fixed(instant).hashCode());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this would compile.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was expecting the GitHub Action to pickup coding and test issues (since my dev setup can't compile or run tests). But it seems it doesn't. do that. The latest commit should at least compile as I copied the test class to another IDE location, but I have no way of knowing if the tests pass.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub Actions just verifies build and tier1 testing, while java.time tests are in tier2, so it won't catch the failure. The test case still does not compile without the import java.time.InstantSource statement. I've verified the test passed with that fix.

import java.time.Clock;
import java.time.Duration;
import java.time.Instant;
import java.time.ZoneId;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs import java.time.InstantSource;

@naotoj
Copy link
Member

naotoj commented May 19, 2021

Another test started failing with your fix. Apparently, the following piece in java/time/test/java/time/TestClock_System.java throws ExceptionInInitializerError.

        static {
            try {
                offsetField = Class.forName("java.time.Clock$SystemClock").getDeclaredField("offset");
                offsetField.setAccessible(true);
            } catch (ClassNotFoundException | NoSuchFieldException ex) {
                throw new ExceptionInInitializerError(ex);
            }
        }

@forax
Copy link
Member

forax commented May 19, 2021

It's a side effect of JEP 403, i believe

@naotoj
Copy link
Member

naotoj commented May 19, 2021

The Error was caused by the movement of the field offset, from inner SystemClock class to Clock.

@forax
Copy link
Member

forax commented May 19, 2021

my bad

@jodastephen
Copy link
Contributor Author

I've made the obvious changes to fix the offset reflection. However it does now mean that the offset is being altered for a singleton where previously it would have only affected Clock.systemUtc(). Is the test class spun up in its own JVM process? Just want to make sure that manipulating the singleton clock won't impact other tests.

@naotoj
Copy link
Member

naotoj commented May 19, 2021

I believe that the default execution mode is agentvm so it will leave unnecessary side effects. To make it run in othervm mode, possibly you will need to tweak TEST.properties (not tried though).

Copy link
Member

@naotoj naotoj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Jun 3, 2021
@openjdk
Copy link

openjdk bot commented Jun 3, 2021

@jodastephen 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:

8266846: Add java.time.InstantSource

Reviewed-by: rriggs, naoto, darcy

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 378 new commits pushed to the master branch:

  • 7e41ca3: 8266957: SA has not followed JDK-8220587 and JDK-8224965
  • 6ff978a: 8267204: Expose access to underlying streams in Reporter
  • 76b54a1: 8263512: [macos_aarch64] issues with calling va_args functions from invoke_native
  • 4e6748c: 8267687: ModXNode::Ideal optimization is better than Parse::do_irem
  • 48dc72b: 8268272: Remove JDK-8264874 changes because Graal was removed.
  • 20b6312: 8268151: Vector API toShuffle optimization
  • 64ec8b3: 8212155: Race condition when posting dynamic_code_generated event leads to JVM crash
  • cd0678f: 8199318: add idempotent copy operation for Map.Entry
  • b27599b: 8268222: javax/xml/jaxp/unittest/transform/Bug6216226Test.java failed, cannot delete file
  • 59a539f: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries
  • ... and 368 more: https://git.openjdk.java.net/jdk/compare/0cc7833f3d84971dd03a9a620585152a6debb40e...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@RogerRiggs, @plevart, @naotoj, @jddarcy) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jun 3, 2021
@jodastephen
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Jun 5, 2021
@openjdk
Copy link

openjdk bot commented Jun 5, 2021

@jodastephen
Your change (at version d564956) is now ready to be sponsored by a Committer.

@RogerRiggs
Copy link
Contributor

/sponsor

@openjdk openjdk bot closed this Jun 5, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 5, 2021
@openjdk
Copy link

openjdk bot commented Jun 5, 2021

@RogerRiggs @jodastephen Since your change was applied there have been 379 commits pushed to the master branch:

  • 7f55dc1: 8179880: Refactor javax/security shell tests to plain java tests
  • 7e41ca3: 8266957: SA has not followed JDK-8220587 and JDK-8224965
  • 6ff978a: 8267204: Expose access to underlying streams in Reporter
  • 76b54a1: 8263512: [macos_aarch64] issues with calling va_args functions from invoke_native
  • 4e6748c: 8267687: ModXNode::Ideal optimization is better than Parse::do_irem
  • 48dc72b: 8268272: Remove JDK-8264874 changes because Graal was removed.
  • 20b6312: 8268151: Vector API toShuffle optimization
  • 64ec8b3: 8212155: Race condition when posting dynamic_code_generated event leads to JVM crash
  • cd0678f: 8199318: add idempotent copy operation for Map.Entry
  • b27599b: 8268222: javax/xml/jaxp/unittest/transform/Bug6216226Test.java failed, cannot delete file
  • ... and 369 more: https://git.openjdk.java.net/jdk/compare/0cc7833f3d84971dd03a9a620585152a6debb40e...master

Your commit was automatically rebased without conflicts.

Pushed as commit 6c838c5.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated