Skip to content

SelfAwareJoinDecider: remove infinite timeout on http lookup - #908

Open
pjfanning wants to merge 6 commits into
apache:mainfrom
pjfanning:decider
Open

SelfAwareJoinDecider: remove infinite timeout on http lookup#908
pjfanning wants to merge 6 commits into
apache:mainfrom
pjfanning:decider

Conversation

@pjfanning

@pjfanning pjfanning commented Jul 30, 2026

Copy link
Copy Markdown
Member

Motivation

SelfAwareJoinDecider.selfContactPoint awaited ClusterBootstrap(system).selfContactPoint with Duration.Inf. The comment justified that with "the future has a timeout", which is only true once ClusterBootstrap.start() has run: ensureSelfContactPoint() schedules the timer that fails the promise, and it is called from start() alone. Bind the bootstrap routes manually and the promise is never completed by anything, so the await never returns.

Two further problems came out of review:

  • The wait and the timer that makes it safe are coupled — the wait is only correct while it outlasts the timer — but they were separate literals in separate files, with nothing expressing the relationship. Raising the timer would silently make the decider time out first, replacing the deliberate "'Bootstrap.selfContactPoint' was NOT set, but is required..." error with a bare TimeoutException from somewhere else.
  • Caching it as a lazy val does not fix the repeated-blocking case, because a lazy val re-runs its initialiser after a throw. In exactly the scenario this PR exists for, every canJoinSelf call would block a dispatcher thread for the full timeout again — Duration.Inf parked one thread once, this parks one per probe.

Modification

  • Move the timer's duration to ClusterBootstrap.SelfContactPointTimeout and derive the decider's wait from it, behind a protected def selfContactPointTimeout that a test can override. The effective value is unchanged.
  • Cache the resolved contact point in an AtomicReference. A failure is deliberately not cached, so a contact point set later is still picked up, but the blocking wait is paid at most once: reaching the timeout means nothing will complete the promise, so subsequent callers check it without blocking and fail fast until it does complete.
  • Resolve against the promise directly instead of mapping it first, so the already-completed case needs no dispatcher hop and Future#value is meaningful.
  • canJoinSelf reuses the self it already resolved instead of asking again.

Result

The two timeouts cannot drift apart. An unset contact point costs one blocking wait rather than one per probe, and is still picked up if it arrives late.

Tests

  • sbt "management-cluster-bootstrap/test" — 54 succeeded, 0 failed (48 before this PR).
  • New SelfContactPointResolutionSpec covers resolution, caching, the timeout, the block-once behaviour, setting a contact point after a timeout, and the ordering between the two timeouts.
  • Directional: with the block-once guard removed so every call waits, "block for the timeout only once, then fail fast" fails with 505796658 nanoseconds was not less than 250 milliseconds.
  • sbt "management-cluster-bootstrap/mimaReportBinaryIssues" — success.
  • sbt "management-cluster-bootstrap/scalafmtCheck" "management-cluster-bootstrap/Test/scalafmtCheck" and +headerCheckAll — clean.

References

None - found by review of SelfAwareJoinDecider

@pjfanning
pjfanning requested review from He-Pin, Philippus and raboof July 30, 2026 08:17
pjfanning added a commit to pjfanning/incubator-pekko-management that referenced this pull request Aug 25, 2026
Motivation:
Follow-up on the review of this branch.

The 30 second wait added here and the 10 second timer in
`ClusterBootstrap.ensureSelfContactPoint` are coupled - the wait is only correct
while it outlasts the timer that completes the promise - but nothing expressed
that. Raising the timer would silently make the decider time out first, replacing
the deliberate "'Bootstrap.selfContactPoint' was NOT set" error with a bare
TimeoutException from elsewhere.

The `lazy val` re-runs its initialiser after a throw, so in the case this change
exists for - `start()` never ran, so nothing ever completes the promise - every
`canJoinSelf` call blocks a dispatcher thread for the full timeout again. The
previous `Duration.Inf` parked one thread once; this parks one per probe, for as
long as the contact point stays unset.

There was no test. `SelfAwareJoinDeciderSpec` only covers the path where the
contact point has already been set, and a 30 second hardcoded timeout cannot be
exercised in a test anyway.

Modification:
Move the timer's duration to `ClusterBootstrap.SelfContactPointTimeout` and
derive the decider's wait from it, behind a `protected def` a test can override.

Replace the `lazy val` with an `AtomicReference` that caches a resolved value.
A failure is deliberately not cached, so a contact point set later is still
picked up, but the blocking wait is paid at most once: reaching the timeout means
the promise has nothing to complete it, so later callers check the promise
without blocking and fail fast until it does complete.

Resolve against the promise directly rather than mapping it first, so that the
already-completed case needs no dispatcher hop and `value` is meaningful.

Result:
The two timeouts cannot drift apart. An unset contact point costs one blocking
wait rather than one per probe, and is still picked up if it arrives late.

Tests:
- sbt "management-cluster-bootstrap/test" - 54 succeeded, 0 failed (48 before)
- New SelfContactPointResolutionSpec covers resolution, caching, the timeout, the
  block-once behaviour, late setting after a timeout, and the ordering between
  the two timeouts
- Directional: with the block-once guard removed so that every call waits, "block
  for the timeout only once, then fail fast" FAILS with "505796658 nanoseconds
  was not less than 250 milliseconds"
- sbt "management-cluster-bootstrap/mimaReportBinaryIssues" - success
- sbt "management-cluster-bootstrap/scalafmtCheck"
  "management-cluster-bootstrap/Test/scalafmtCheck", "+headerCheckAll" - clean

References:
Refs apache#908
pjfanning added a commit to pjfanning/incubator-pekko-management that referenced this pull request Aug 25, 2026
Motivation:
ClusterBootstrap.SelfContactPointTimeout is private[bootstrap], so scaladoc
cannot resolve a [[...]] link to it and unidoc fails the Docs compile job with
'Could not find any member to link'.

Modification:
Refer to it as code rather than as a link, in both places.

Result:
Scaladoc generates again.

Tests:
- sbt "unidoc; docs/paradox" - success, which is the exact command the Docs
  compile job runs

References:
Refs apache#908
Motivation:
Follow-up on the review of this branch.

The 30 second wait added here and the 10 second timer in
`ClusterBootstrap.ensureSelfContactPoint` are coupled - the wait is only correct
while it outlasts the timer that completes the promise - but nothing expressed
that. Raising the timer would silently make the decider time out first, replacing
the deliberate "'Bootstrap.selfContactPoint' was NOT set" error with a bare
TimeoutException from elsewhere.

The `lazy val` re-runs its initialiser after a throw, so in the case this change
exists for - `start()` never ran, so nothing ever completes the promise - every
`canJoinSelf` call blocks a dispatcher thread for the full timeout again. The
previous `Duration.Inf` parked one thread once; this parks one per probe, for as
long as the contact point stays unset.

There was no test. `SelfAwareJoinDeciderSpec` only covers the path where the
contact point has already been set, and a 30 second hardcoded timeout cannot be
exercised in a test anyway.

Modification:
Move the timer's duration to `ClusterBootstrap.SelfContactPointTimeout` and
derive the decider's wait from it, behind a `protected def` a test can override.

Replace the `lazy val` with an `AtomicReference` that caches a resolved value.
A failure is deliberately not cached, so a contact point set later is still
picked up, but the blocking wait is paid at most once: reaching the timeout means
the promise has nothing to complete it, so later callers check the promise
without blocking and fail fast until it does complete.

Resolve against the promise directly rather than mapping it first, so that the
already-completed case needs no dispatcher hop and `value` is meaningful.

Result:
The two timeouts cannot drift apart. An unset contact point costs one blocking
wait rather than one per probe, and is still picked up if it arrives late.

Tests:
- sbt "management-cluster-bootstrap/test" - 54 succeeded, 0 failed (48 before)
- New SelfContactPointResolutionSpec covers resolution, caching, the timeout, the
  block-once behaviour, late setting after a timeout, and the ordering between
  the two timeouts
- Directional: with the block-once guard removed so that every call waits, "block
  for the timeout only once, then fail fast" FAILS with "505796658 nanoseconds
  was not less than 250 milliseconds"
- sbt "management-cluster-bootstrap/mimaReportBinaryIssues" - success
- sbt "management-cluster-bootstrap/scalafmtCheck"
  "management-cluster-bootstrap/Test/scalafmtCheck", "+headerCheckAll" - clean

References:
Refs apache#908
Motivation:
ClusterBootstrap.SelfContactPointTimeout is private[bootstrap], so scaladoc
cannot resolve a [[...]] link to it and unidoc fails the Docs compile job with
'Could not find any member to link'.

Modification:
Refer to it as code rather than as a link, in both places.

Result:
Scaladoc generates again.

Tests:
- sbt "unidoc; docs/paradox" - success, which is the exact command the Docs
  compile job runs

References:
Refs apache#908
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants