pjfanning opened a new pull request, #956: URL: https://github.com/apache/pekko-management/pull/956
### Motivation Follow-up to #906. **1. `lookup-parallelism = 0` hangs the lookup forever.** `boundedTraverse` splits by `settings.parallelism`, and nothing validates it: ```scala val (batch, rest) = remaining.splitAt(settings.parallelism) Future.traverse(batch)(f).flatMap(results => loop(rest, results.reverse ++ acc)) ``` `splitAt(n)` with `n <= 0` returns `(empty, remaining)`, so `Future.traverse(empty)` completes immediately and `loop` recurses with **identical arguments**. Running the merged function verbatim: ``` parallelism = 2 -> List(A, B, C, D, E) parallelism = 0 -> TimeoutException after 4861031 loop iterations ``` A user who reads `lookup-parallelism = 0` as "unbounded" gets a future that never completes and pins a core, rather than a configuration error. **2. The `ca-path` `FileInputStream` is never closed** (`ConsulServiceDiscovery.scala:59`) — a file descriptor leak, in the PR whose headline item was fixing a resource leak. **3. The TLS branch supplies an `SSLContext` but no trust manager.** `Consul$Builder.addSslSocketFactory` falls back to `TrustManagerUtils.getDefaultTrustManager()` when none is given, so the built client uses the **default JVM** trust manager for chain cleaning while the handshake uses the custom CA context. I had initially called this latent; it is measurable — the client ends up accepting **144** accepted issuers (the whole JDK trust store) instead of the 1 CA configured via `ca-path`. **4. `consul.destroy()` runs on `system.dispatcher`.** The PR's own second item moved blocking work off `system.dispatcher` and introduced `blockingEc`; the shutdown task added by its first item then didn't use it. `destroy()` shuts down OkHttp's connection pool and executor service, which blocks. **5. The timeout comment claims something untrue.** Nothing in the new code cancels the in-flight Consul requests when the timeout fires. What the `Promise` pattern actually buys is cancelling the *scheduled timeout* once the lookup completes — a real improvement, described as a different one. **6. No tests, and no docs.** 133 added lines with zero tests, and seven new configuration keys that never reached `docs/`. ### Modification - `ConsulSettings` requires `lookup-parallelism > 0`; `boundedTraverse` takes the parallelism as a parameter and guards it too. - CA stream closed with `Using.resource`; the CA's `X509TrustManager` is passed to the builder alongside the `SSLContext`. - `destroy()` moved onto `blockingEc`. - Timeout comment replaced with what the code does. - The Consul client is now built lazily behind an `AtomicReference` populated only after it exists, so a configured-but-unused discovery instance creates nothing and shutdown never forces the lazy val — the shape #907 introduced for the AWS clients. Creation sits behind `private[consul] createConsulClient()` so tests can observe it. - Dropped the redundant `immutable.Seq(targets: _*)` copy of an already-immutable `Seq`. - Documented the configuration keys #906 added, plus 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 leak, blocking shutdown work stays off the default dispatcher, and the settings are discoverable from the docs. ### Tests New `ConsulServiceDiscoveryInternalsSpec` — 9 succeeded, 0 failed. It needs no Consul container (the kiwiproject builder does not ping by default, so the client builds offline), so it runs in ordinary CI alongside the existing container-based `ConsulDiscoverySpec`. Directional checks: - With `ConsulSettings.scala` reverted to `main` — *"should reject a lookup-parallelism of 0"* and *"should reject a negative lookup-parallelism"* both **FAILED**. - With the `withTrustManager` call removed — *"should trust only the configured CA certificate when ca-path is set"* **FAILED**: `had length 144 instead of expected length 1`. Also: - `sbt "discovery-consul/mimaReportBinaryIssues"` — success. - `sbt "discovery-consul/scalafmt" "discovery-consul/Test/scalafmt"` and `sbt headerCreateAll` — clean. - `ConsulDiscoverySpec` not run locally — it needs a Consul testcontainer; left to CI. `src/test/resources/consul-test-ca.crt` is a self-signed certificate generated for this test. Only the certificate is checked in — no private key. ### References Refs #906 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
