The GitHub Actions job "Scalafmt" on pekko-management.git/consul-follow-up has succeeded. Run started by GitHub user pjfanning (triggered by pjfanning).
Head commit for run: 04f07f40d0552eb58b6ee0ae72587321e98c8607 / PJ Fanning <[email protected]> Validate Consul lookup-parallelism and tidy the client lifecycle Motivation: `boundedTraverse`, added in #906, splits the work by `pekko.discovery.pekko-consul.lookup-parallelism`, and nothing validates that value. `splitAt(n)` with `n <= 0` returns `(empty, remaining)`, so the batch is empty, `Future.traverse` of it completes immediately, and `loop` recurses with an unchanged `remaining`/`acc` pair. Running the merged function verbatim with `lookup-parallelism = 0` and five items spins through 4,861,031 iterations in five seconds without completing - a lookup that never finishes and pins a core, rather than a configuration error. Four smaller problems came in with the same PR. The CA certificate stream in the TLS branch is never closed, which leaks a file descriptor. The TLS branch passes an `SSLContext` but no trust manager, and the Consul builder then falls back to the default JVM trust manager: the built client accepts all 144 CAs in the JDK trust store for chain cleaning instead of only the CA configured via `ca-path`. The `consul-close` shutdown task runs the blocking `consul.destroy()` on `system.dispatcher` even though the same PR introduced `blockingEc` for exactly this kind of work. And the comment above the new `Promise`-based timeout claims it avoids leaking Consul HTTP connections when the timeout fires, which it does not - nothing cancels the requests already in flight. What it does do is cancel the scheduled timeout once the lookup completes. #906 also added seven configuration keys without documenting any of them. Modification: Require `lookup-parallelism > 0` in `ConsulSettings`, and guard `boundedTraverse` itself, which now takes the parallelism as a parameter rather than reading the settings directly. Close the CA stream with `Using.resource`, and pass the CA's `X509TrustManager` to the builder alongside the `SSLContext`. Move `destroy()` onto `blockingEc`. Replace the timeout comment with what the code actually does. Build the Consul client lazily behind an `AtomicReference` that is populated only after the client exists, so a configured-but-unused discovery instance creates nothing and shutdown never forces the lazy val - the same shape #907 introduced for the AWS clients. Client creation moves behind `private[consul] createConsulClient()` so tests can observe it. Drop the redundant `immutable.Seq(targets: _*)` copy of an already-immutable `Seq`. Document the configuration keys added by #906, and note the client lifecycle. Result: A non-positive `lookup-parallelism` fails at startup with a message naming the key, instead of hanging the first lookup. A client configured with `ca-path` trusts only that CA. No file descriptor is leaked, blocking shutdown work stays off the default dispatcher, and the settings are discoverable from the docs. Tests: - sbt "discovery-consul/testOnly *ConsulServiceDiscoveryInternalsSpec" - 9 succeeded, 0 failed (new spec; needs no Consul container) - Same spec with ConsulSettings.scala reverted to main - "should reject a lookup-parallelism of 0" and "should reject a negative lookup-parallelism" both FAILED - Same spec with the withTrustManager call removed - "should trust only the configured CA certificate when ca-path is set" FAILED, 144 accepted issuers instead of 1 - sbt "discovery-consul/mimaReportBinaryIssues" - success - sbt "discovery-consul/scalafmt" "discovery-consul/Test/scalafmt", sbt headerCreateAll - clean - ConsulDiscoverySpec not run - it needs a Consul testcontainer, left to CI References: Refs #906 Report URL: https://github.com/apache/pekko-management/actions/runs/32875294932 With regards, GitHub Actions via GitBox --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
