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

[vy]

Introduce necessary fixes to address exceptions thrown when excessive Durations are provided to Duration-accepting HttpClient public APIs.

---

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-8368528: HttpClient.Builder.connectTimeout should accept arbitrarily large values (Bug - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27973

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back vyazici! 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]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@vy 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.

[mlbridge]

Webrevs

• 01: Full - Incremental (856aea73)
• 00: Full (c2179b8a)

[pavelrappo]

We might soon have saturating addition functionality in java.time.Instant; see: #27549

I note that jdk.internal.net.http.common.Deadline also wants to have saturating subtraction, and I wonder if that's really needed. It seems that the two usages of the minus method in the codebase can be reimplemented alternatively. In which case Deadline could delete minus.

Furthermore, if there's no need for saturating subtraction, do we need the Deadline class? What does it provide, that Instant does not?

[vy]

We might soon have saturating addition functionality in java.time.Instant; see: #27549

Great tip! 💯 I will hold this PR until #27549 gets merged, and use Instant::plusSaturated in Deadline::plus* and ::minus methods.

I note that jdk.internal.net.http.common.Deadline also wants to have saturating subtraction, and I wonder if that's really needed. It seems that the two usages of the minus method in the codebase can be reimplemented alternatively. In which case Deadline could delete minus.

I also have my reservations regarding the rich, yet seldom used API surface of Deadline. But revamping it is out of the scope of this work.

Furthermore, if there's no need for saturating subtraction, do we need the Deadline class? What does it provide, that Instant does not?

In short, Instant is not necessarily generated using a monotonically-increasing InstantSource. Deadline is introduced to avoid that ambiguity and guaranteed to be always monotonically-increasing. See this conversation for details.

[pavelrappo]

We might soon have saturating addition functionality in java.time.Instant; see: #27549

Great tip! 💯 I will hold this PR until #27549 gets merged, and use Instant::plusSaturated in Deadline::plus* and ::minus methods.

One problem for this PR is that the proposed Instant functionality in that PR will only work with Duration not TemporalAmount. Another problem is that you cannot implement saturating subtraction based on saturating addition here. If you are thinking along these lines, then it will fail if amountToSubtract is the minimum value for Duration:

deadline.plus(amountToSubtract.negated())

Now, I understand that in your case you will never have negative duration, let alone such extremely negative one. But it would still be good to be robust, especially if it also involves less code.

Deadline.minus seems to be used twice. Both times it is used for a comparison like this:

t1 - dt < t0

To avoid subtraction, rearrange the terms. Different rearrangements enable different options, but either option is fine:

• t1 - t0 < dt (compare durations using Instant.until/Duration.between)
• t1 < t0 + dt (compare instants using future Instant.plusSaturating)

I note that jdk.internal.net.http.common.Deadline also wants to have saturating subtraction, and I wonder if that's really needed. It seems that the two usages of the minus method in the codebase can be reimplemented alternatively. In which case Deadline could delete minus.

I also have my reservations regarding the rich, yet seldom used API surface of Deadline. But revamping it is out of the scope of this work.

Furthermore, if there's no need for saturating subtraction, do we need the Deadline class? What does it provide, that Instant does not?

In short, Instant is not necessarily generated using a monotonically-increasing InstantSource. Deadline is introduced to avoid that ambiguity and guaranteed to be always monotonically-increasing. See this conversation for details.

Okay, so you want your source of ticks to be exclusive and monotonic, neither of which could be guaranteed without introducing a few specialised types. Got it.

[pavelrappo]

@vy, Instant.plusSaturating is in the mainline: 2758c6f

[vy]

I discussed this matter internally with @pavelrappo and @dfuch, and decided to keep the code as is, and not use the recently introduced Instant::plusSaturating(Duration), because:

1. This will result in extra work for backports.
2. Not all Deadline methods can take advantage of Instant::plusSaturating, e.g., ::between, ::until, and ::plus(long,TemporalUnit). Instead of some using Instant::plusSaturating and some catching DateTimeException | ArithmeticException, settle on a single approach to ease the cognitive load.

[dfuch]

Do we need to change this method? I would just revert them.

[vy]

Duration#between(Temporal,Temporal) can throw DateTimeException and ArithmeticException, but not in our case due to the reason I elaborated in the comment. In this change, I've removed these two exceptions from the Javadoc, since they cannot happen.