RFR: 8309939: HttpClient should not use Instant.now() as Instant source for deadlines [v2]

Jaikiran Pai jpai at openjdk.org
Wed Jun 14 11:21:00 UTC 2023


On Wed, 14 Jun 2023 11:02:04 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

>> 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.
>
> Daniel Fuchs has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision:
> 
>  - Merge branch 'master' into TimeSource
>  - 8309939

src/java.net.http/share/classes/jdk/internal/net/http/ConnectionPool.java line 132:

> 130:     }
> 131: 
> 132:     ConnectionPool(long clientId, TimeSource timeSource) {

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()`?

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

> 74:                 // The use of Integer.MAX_VALUE is arbitrary.
> 75:                 // Any value not too close to Long.MAX_VALUE
> 76:                 // would do.

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.

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

> 80:         }
> 81: 
> 82:         // @ForceInline

Should these couple of commented out `@ForceInline` be deleted?

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/14450#discussion_r1229428984
PR Review Comment: https://git.openjdk.org/jdk/pull/14450#discussion_r1229432322
PR Review Comment: https://git.openjdk.org/jdk/pull/14450#discussion_r1229430561


More information about the net-dev mailing list