HwangRock opened a new pull request, #5300: URL: https://github.com/apache/zeppelin/pull/5300
### What is this PR for? `ZeppelinWebSocketClient.connect()` blocks forever when the websocket handshake fails. It waits on `connectLatch.await()` with no timeout, and the latch is only counted down in the success callback (`onConnect`). Pre-handshake failures — connection refused, TLS error, rejected upgrade — are not delivered through the annotated callbacks at all; Jetty completes the `CompletableFuture` returned by `WebSocketClient.connect()` exceptionally instead. The original code discards that future, so the failure never reaches the waiting thread and `connect()` hangs. This affects any `zeppelin-client` (`ZSession`) user that passes a `MessageHandler` to `start()`/`reconnect()`: if the target Zeppelin server is down or unreachable, the calling thread blocks indefinitely. Fix: wait on the future with a bounded timeout and surface handshake failures as `IOException`. On failure, stop the Jetty client so its threads are not leaked. `connectLatch` is unused after this change, so it is removed together with its unused `getConnectLatch()` getter. Note on approach: adding `connectLatch.countDown()` to `onError` does not fix this. Against jetty-websocket 11, pre-handshake failures reach only the future, never `@OnWebSocketError`, so the latch would still never be released. The future is the only path that carries the failure. ### What type of PR is it? Bug Fix ### Todos * [ ] - Follow-up: make the connect timeout configurable via `ClientConfig` (currently a 30s constant) ### What is the Jira issue? * https://issues.apache.org/jira/browse/ZEPPELIN-6531 ### How should this be tested? Added `ZeppelinWebSocketClientTest` (the first unit test in `zeppelin-client`): connecting to a closed port must fail within a bounded time instead of hanging. Also verified end-to-end against a docker-exposed port. Ran `nginx` on a port, which accepts the TCP connection but rejects the websocket upgrade (HTTP 404 on `/ws`), and pointed the client at both that port and a closed port. Before (original code, same docker port): ``` [INFO] Running org.apache.zeppelin.client.websocket.ManualE2EVerifyTest ``` Hangs here — no result line. The forked surefire JVM stayed alive past 45s and had to be killed. After (this PR): ``` [E2E] closed-port(1) failed-fast in 295ms -> IOException: Failed to establish websocket connection to ws://127.0.0.1:1/ws [E2E] open-port-non-ws(18080) failed-fast in 45ms -> IOException: Failed to establish websocket connection to ws://127.0.0.1:18080/ws ``` The docker probe was a throwaway check and is not part of the PR; the committed test uses a closed port and needs no docker. ### Screenshots (if appropriate) ### Questions: * Does the license files need to update? No. * Is there breaking changes for older versions? `getConnectLatch()` is removed. It is public but has no callers in the repo, and it only exposed `connect()`'s internal latch, which no longer exists. * Does this needs documentation? No. -- 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]
