Skip to content

Fix WebSocket client wedging that made OpenICFWebSocketTest flaky#104

Open
vharseko wants to merge 2 commits into
OpenIdentityPlatform:masterfrom
vharseko:fix-websocket-client-permit-leak
Open

Fix WebSocket client wedging that made OpenICFWebSocketTest flaky#104
vharseko wants to merge 2 commits into
OpenIdentityPlatform:masterfrom
vharseko:fix-websocket-client-permit-leak

Conversation

@vharseko

Copy link
Copy Markdown
Member

Fixes #102

OpenICFWebSocketTest.testRemoteConnectorInfoManager hung for a full 5 minutes on the macOS/JDK 26 CI runner and killed the whole connector-server-grizzly fork ("There was a timeout in the fork"). The root causes are unrecoverable states in the WebSocket client, detailed in #102.

Production code (connector-framework-server)

  • ClientRemoteConnectorInfoManager
    • onClosed: always release the permit held by the closing connection. The inverted availableConnectionPermitCount < 1 guard skipped the release exactly when the closing connection held the last permit, leaving a single-connection client permanently unable to reconnect (heartbeat connect(false) and the on-demand path are both permit-gated). tryReleaseConnectionPermit() is already capped at permittedConnectionCount, so it cannot over-credit.
    • Releasing in the failed() callback was deliberately not added: per Grizzly 4.0.2 TCPNIOConnectorHandler sources, every failure path after preConfigure also closes the connection, so the close listener fires and a failed() release would double-credit.
    • Heartbeat self-heal for the remaining case (connect attempt failing before the Grizzly Connection exists, e.g. fd exhaustion — no close listener to release the permit): if there is no live connection, no free permit and no connect attempt for 30 s, restore the lost permit.
  • WebSocketConnectionGroup / WebSocketConnectionHolder: the initial CONNECTOR_INFO request was sent from inside handshake(), before the holder assigned its RemoteOperationContext — a fast response was dispatched into socket.getRemoteConnectionContext() == null → NPE, silently dropping the connector info with no retry (the retry chain only reacts to promise failure). The request is now sent from a new handshakeComplete() hook invoked by receiveHandshake after the context is stored; this fixes the client, the Grizzly server and the Jetty server holders at once, since all three assign the context after handshake() returns. Also guarded trySubmitRequest() returning null (allowed by its contract) in the initial request and its retry.

Tests / infra

  • OpenICFWebSocketTest: the poll loop was dead code — getOrThrowUninterruptibly never returns null, so the test was a single 5-minute blocking wait. Replaced with real polling: 5 × 20-second attempts.
  • grizzly / jetty poms: forkedProcessTimeoutInSeconds 300 → 900. The in-test wait (300 s) was equal to the fork timeout, so any single hang was converted into a fork kill for the whole module instead of one failed test.

Verification

  • connector-framework-server: 24/24 pass
  • connector-server-grizzly (incl. OpenICFWebSocketTest): 32/32 pass, two consecutive runs (~51 s each vs 350 s with the hang)
  • connector-server-jetty: 30/30 pass
  • javadoc:javadoc (strict doclint): clean

…enIdentityPlatform#102)

- ClientRemoteConnectorInfoManager: always release the connection permit
  in onClosed - the inverted guard leaked the last permit of a
  single-connection client, leaving it unable to reconnect forever;
  self-heal a permit lost by a connect attempt that failed before the
  Grizzly Connection was created
- WebSocketConnectionGroup/Holder: send the initial CONNECTOR_INFO
  request only after the holder has stored its RemoteOperationContext,
  so an early response can no longer be dropped with an NPE; guard
  trySubmitRequest returning null
- OpenICFWebSocketTest: replace the unreachable 5-minute poll loop with
  real 20-second polling attempts
- grizzly/jetty poms: raise forkedProcessTimeoutInSeconds to 900 so a
  single hung test fails itself instead of killing the whole fork
@vharseko vharseko added bug Something isn't working tests Test additions or fixes framework OpenICF-java-framework labels Jul 17, 2026
@vharseko
vharseko requested a review from maximthomas July 17, 2026 16:13

@maximthomas maximthomas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-heal can over-credit a permit for an in-flight connection (medium)

ClientRemoteConnectorInfoManager.java:209-219lastConnectAttempt is stamped before connectionHandler.connect(...) (:385), but the holder only reaches privateConnections in thenOnResult
(:401-410), i.e. after the WebSocket handshake. If TCP connects but the handshake stalls past 30s (the same failure class this PR is about), the heartbeat releases a permit a live pending connection
still holds — then that connection closes and its listener releases again. The cap bounds available, not available + live connections, so you can transiently exceed expectedConnectionCount.

Suggested: stamp in preConfigure() (where the close listener is registered) and heal only when the last attempt produced no Connection:

&& lastConnectionCreated.get() < lastConnectAttempt.get()

That matches the invariant the comment describes, and a same-ms tie correctly declines to heal. Note the heal still only covers privateConnections.isEmpty(), so a permit leaked while other connections
are live never recovers — fine as a narrow fix, just not a complete one.

Nits

  • receivedConnectorInfo is still non-volatile: written from callback threads (WebSocketConnectionGroup.java:139), read from the scheduler (:354). Pre-existing, but you're adding an AtomicBoolean
    on the next line — cheap to fix while here.
  • The CONNECTOR_INFO factory is now built in three places; a small private helper would dedupe.

…onnection

- Heal only when the last connect attempt never created a Connection
  (lastConnectionCreated < lastConnectAttempt): if preConfigure ran, the
  close listener is registered and will release the permit itself, so
  healing would over-credit while the connection is pending its handshake.
- Make receivedConnectorInfo volatile: written from transport callbacks,
  read by the scheduler in checkIsActive().
- Dedupe the CONNECTOR_INFO request factory into a helper.
@vharseko

Copy link
Copy Markdown
Member Author

@maximthomas thanks for the careful review — all three points addressed in 0fd9082.

Self-heal over-credit — fixed as you suggested: a new lastConnectionCreated timestamp is stamped in preConfigure() (right after the close listener is registered), and the heal now additionally requires lastConnectionCreated < lastConnectAttempt.

One detail from the Grizzly 4.0.2 sources that makes this guard tighter than it may look: in TCPNIOConnectorHandler.connectAsync the Connection is created and preConfigure() runs synchronously, before socketChannel.connect(remoteAddress) is even initiated, and any later exception in that method calls newConnection.closeSilently() (which fires our close listener). So created < attempt is effectively equivalent to "the last attempt has no close listener": the only orphan path is openSocketChannel() / obtainNIOConnection() throwing (e.g. fd exhaustion) — exactly the case the heal targets. Your stalled-handshake scenario now correctly declines to heal (the pending connection's close listener will release the permit), and the same-ms tie declines as well.

Agreed the heal remains narrow (privateConnections.isEmpty() only); a permit leaked while other connections are live is left out deliberately. The remaining theoretical wedge — a pending connection whose WS handshake stalls forever without the socket ever closing — would need a handshake timeout that closes the raw connection; happy to do that as a follow-up if you think it's worth it.

Nits — both taken: receivedConnectorInfo is now volatile (written from transport callbacks, read by the scheduler in checkIsActive()), and the three CONNECTOR_INFO factory sites are deduped into connectorInfoRequestFactory().

Verification re-run: connector-framework-server 24/24, connector-server-grizzly 32/32 (incl. OpenICFWebSocketTest), connector-server-jetty 30/30, strict javadoc:javadoc clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working framework OpenICF-java-framework tests Test additions or fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky OpenICFWebSocketTest.testRemoteConnectorInfoManager: client connection permit leak causes 5-min hang and Surefire fork timeout

2 participants