8309939: HttpClient should not use Instant.now() as Instant source for deadlines #14450

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.

Sign up for GitHub

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

Jump to bottom

Closed

dfuch wants to merge 8 commits into openjdk:master from dfuch:TimeSource

Member

dfuch commented Jun 13, 2023 •

edited by openjdk bot

Loading

The HttpClient uses Instant.now() to create deadlines for timeouts. This could have undesirable effects since Instant.now() is linked to the wall clock, which is not monotonic. This fix changes the HttpClient to use a monotonic instant source based on System.nanoTime() for the purpose of setting and comparing deadlines.

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

Issue

JDK-8309939: HttpClient should not use Instant.now() as Instant source for deadlines (Bug - P4)

Reviewers

Daniel Jeliński (@djelinski - Reviewer) ⚠️ Review applies to e5f6cd60

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14450

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

Using diff file

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

Webrev

Link to Webrev Comment

1ab59db

bridgekeeper bot commented Jun 13, 2023

👋 Welcome back dfuchs! 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 bot changed the title ~~8309939~~ 8309939: HttpClient should not use Instant.now() as Instant source for deadlines

openjdk bot added the rfr label

openjdk bot commented Jun 13, 2023

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

net

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 bot added the net label

mlbridge bot commented Jun 13, 2023 •

edited

Loading

Webrevs


          Merge branch 'master' into TimeSource

e30a683

jaikiran reviewed

View reviewed changes

src/java.net.http/share/classes/jdk/internal/net/http/ConnectionPool.java Outdated

    
                   * There should be one of these per HttpClient.

                   */

                  private ConnectionPool(String tag) {

                  ConnectionPool(long clientId, TimeSource timeSource) {

Member

jaikiran Jun 14, 2023

Hello Daniel, as far as I can see, no one calls this constructor outside of this very class and this seems to be always passed a constant TimeSource.source(). So maybe we don't have to have a timeSource field in this class and instead just use TimeSource.now() at call sites within this class (and ExpiryList) whenever we want now()?

Member Author

dfuch Jun 14, 2023 •

edited

Loading

This constructor should in fact take an InstantSource, not a TimeSource. It was added in case we wanted to upgrade the whitebox ConnectionPoolTest to create an instance of ConnectionPool with a custom InstantSource.

jaikiran reviewed

View reviewed changes

src/java.net.http/share/classes/jdk/internal/net/http/common/TimeSource.java Outdated

    
                          return now;

                      }

                      // @ForceInline

Member

jaikiran Jun 14, 2023

Should these couple of commented out @ForceInline be deleted?

Member Author

dfuch Jun 14, 2023

Done

jaikiran reviewed

View reviewed changes

src/java.net.http/share/classes/jdk/internal/net/http/common/TimeSource.java Outdated

    
                              // around.

                              // The use of Integer.MAX_VALUE is arbitrary.

                              // Any value not too close to Long.MAX_VALUE

                              // would do.

Member

jaikiran Jun 14, 2023 •

edited

Loading

The Integer.MAX_VALUE gets used in isInWindow() method implementation, through the use of TIME_WINDOW member. Should this comment about Integer.MAX_VALUE instead be moved as a field comment for TIME_WINDOW? That way it's closer to where the Integer.MAX_VALUE is actually used.

Member Author

dfuch Jun 14, 2023

Done

djelinski reviewed

View reviewed changes

src/java.net.http/share/classes/jdk/internal/net/http/common/TimeSource.java Outdated

    
                  private NanoSource localSource = nanoSource;

                  private static final TimeSource SOURCE = new TimeSource();

                  private static final class NanoSource implements InstantSource {

Member

djelinski Jun 14, 2023

This class doesn't need to implement InstantSource

Member Author

dfuch Jun 14, 2023

Good point

src/java.net.http/share/classes/jdk/internal/net/http/common/TimeSource.java Outdated

    
                      }

                      @Override

                      public Instant instant() {

Member

djelinski Jun 14, 2023

This method is not used anywhere

src/java.net.http/share/classes/jdk/internal/net/http/common/TimeSource.java Outdated

    
               * of {@link System#nanoTime()}. This time source has the same property

               * of monotonicity than the {@link System#nanoTime()} it is based on.

               */

              public final class TimeSource implements InstantSource {

Member

djelinski Jun 14, 2023

Given that all instances of TimeSource are essentially the same (through the use of shared nanoSource), can we add a private constructor and make this class a singleton?

Member Author

dfuch Jun 14, 2023

Good point. That's an oversight.


          Review feedback

35da58e

openjdk bot removed the rfr label


          whitespaces

8eeb13f

openjdk bot added the rfr label


          more cleanup

4636ccb

RogerRiggs suggested changes

View reviewed changes

src/java.net.http/share/classes/jdk/internal/net/http/common/TimeSource.java

Comment on lines +35 to +36

    
               * instants returned by this time source solely for the purpose of

               * comparing them with other instants returned by this same time source.

Contributor

RogerRiggs Jun 14, 2023 •

edited

Loading

Do not use Instant as the value from this time source. It is not comparable with the real Instants and would be misleading and cause bugs.

Member Author

dfuch Jun 14, 2023

OK. Here is a version that uses a new class "Deadline" instead of "Instant".

Contributor

RogerRiggs Jun 14, 2023

Wrapping the Instant is safe/fine.
However, it could quite a bit simpler (it seems) to just use the raw long nanoTime values.
Though milliseconds or seconds would be sufficient for the timeouts.
The complexity of the volatile read and needing to reset the nanoSource could be avoided.

Member Author

dfuch Jun 14, 2023 •

edited

Loading

Working with Instant (now Deadline) and Duration is much safer and easier than working with longs representing nano time, especially when you have a sorted list of deadlines. I'd argue that the complexity is when working with raw long nanoTime values.


          Use Deadline rather than Instant

e5f6cd6

djelinski reviewed

View reviewed changes

src/java.net.http/share/classes/jdk/internal/net/http/common/Deadline.java Outdated

    
                  @Override

                  public String toString() {

                      return deadline.toString();

Member

djelinski Jun 16, 2023

I'm not sure about using Instant.toString here; if we ever log a Deadline instance along with the current time and the clock skews, we will need to read the code along with the logs to figure out which date represents a deadline and which one represents an instant.
I guess the problem would also go away if we initialized the first NanoSource with something other than Instant.now().

Member Author

dfuch Jun 16, 2023

I can modify the toString to be

  return "Deadline(" + deadline.toString() + ")";

if you think it's better.

But I do believe that using a fake instant for the origin of the NanoSource would be way more confusing.

Member

djelinski Jun 16, 2023

We can also avoid using that method. I guess we can always deal with it later if it proves to be a problem.

djelinski approved these changes

View reviewed changes

Member

djelinski left a comment

Using nanoTime as @RogerRiggs suggested is worth exploring, but IMO the current version is good enough for internal use.

openjdk bot commented Jun 16, 2023 •

edited

Loading

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

8309939: HttpClient should not use Instant.now() as Instant source for deadlines

Reviewed-by: djelinski

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

d2a858e: 7083187: Class CSS.CssValue is missing implementations of equals() and hashCode()
4229baf: 8310015: ZGC: Unbounded asynchronous unmapping can lead to running out of address space
266f983: 8308855: ARM32: TestBooleanVector crashes after 8300257
6a63bad: 8310191: com/sun/tools/attach/warnings/DynamicLoadWarningTest.java second failure on AIX
6473a7d: 8310107: os::trace_page_sizes_for_requested_size should name alignment as requested page size
02aaab1: 8310126: C1: Missing receiver null check in Reference::get intrinsic
492d25c: 8309601: [JVMCI] AMD64#getLargestStorableKind returns incorrect mask kind
959a61f: 8310259: Pin msys2/setup-msys2 github action to a specific commit
bcc4d36: 8309511: Regression test ExtraImportSemicolon.java refers to the wrong bug
71baf00: 8309605: StubRoutines are not used by SA
... and 47 more: https://git.openjdk.org/jdk/compare/bd79db3930f192f6742e29a63a6d1c3bc3dd3385...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 bot added the ready label

dfuch added 2 commits

June 19, 2023 10:38


          Change Deadline::toString

08f4300


          whitespaces

e512b7c

Member Author

dfuch commented Jun 19, 2023

/integrate

openjdk bot commented Jun 19, 2023

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

7d4b77a: 8304835: jdk/jfr/event/oldobject/TestArrayInformation.java fails with "Could not find event with class ... as (leak) object"
b896e3e: 8310146: Removing unused PerfLongVariant::_sampled
d2a858e: 7083187: Class CSS.CssValue is missing implementations of equals() and hashCode()
4229baf: 8310015: ZGC: Unbounded asynchronous unmapping can lead to running out of address space
266f983: 8308855: ARM32: TestBooleanVector crashes after 8300257
6a63bad: 8310191: com/sun/tools/attach/warnings/DynamicLoadWarningTest.java second failure on AIX
6473a7d: 8310107: os::trace_page_sizes_for_requested_size should name alignment as requested page size
02aaab1: 8310126: C1: Missing receiver null check in Reference::get intrinsic
492d25c: 8309601: [JVMCI] AMD64#getLargestStorableKind returns incorrect mask kind
959a61f: 8310259: Pin msys2/setup-msys2 github action to a specific commit
... and 49 more: https://git.openjdk.org/jdk/compare/bd79db3930f192f6742e29a63a6d1c3bc3dd3385...master

Your commit was automatically rebased without conflicts.

openjdk bot added the integrated label

openjdk bot closed this

openjdk bot removed the ready label

openjdk bot removed the rfr label

openjdk bot commented Jun 19, 2023

@dfuch Pushed as commit f8f8bfb.

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

vy mentioned this pull request

8368528: HttpClient.Builder.connectTimeout should accept arbitrarily large values #27973

Open

3 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels