Don't block IceAdapter.start() on telemetry websocket connect#84
Don't block IceAdapter.start() on telemetry websocket connect#84juerghegglin wants to merge 1 commit intoFAForever:masterfrom
Conversation
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe ChangesAsynchronous Telemetry Connection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…AForever#42) TelemetryDebugger.startupComplete() called websocketClient.connectBlocking() synchronously on the IceAdapter startup thread, with no timeout. Telemetry is a debug-only observability channel - its data flow is strictly outgoing (see ice-adapter/src/main/java/com/faforever/iceadapter/telemetry/, all messages implement OutgoingMessageV1) and no game logic depends on it being connected - so it should never gate peer connectivity. When the telemetry server is silently unreachable (TCP layer up but app layer hung - for example an alive load balancer fronting a dead backend), the WebSocket Upgrade handshake never receives an HTTP 101 response. connectBlocking() then waits until TCP keepalive eventually kills the socket, about two hours later. During that whole window IceAdapter.start() is blocked, the FAF client's adapter-ready orchestration trips its own timeouts, and the user-visible symptom is "the ice adapter doesn't connect to other players". Fix: run the connect on a virtual thread so startup returns immediately. The pre-existing reconnect loop in sendingLoop() already handles transient drops, and queued messages catch up once the socket opens because the messageQueue is unbounded. Also adds the missing return after the InterruptedException catch (previously fell through to sendMessage on an interrupted thread) and re-asserts the interrupt flag, per Java best practice. Fixes FAForever#42
juerghegglin
force-pushed
the
fix/issue-42-async-telemetry-startup
branch
from
6e1e935 to
ca3282c
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Problem
TelemetryDebugger.startupComplete()callswebsocketClient.connectBlocking()synchronously on theIceAdapterstartup thread, with no timeout. Since this is the last thingIceAdapter.start()does, the entire startup is held until that call returns.When the telemetry server is silently unreachable — TCP layer up but application layer hung, a common failure mode where a load balancer fronts a dead backend — the WebSocket Upgrade handshake never receives an HTTP 101 response.
connectBlocking()then waits until TCP keepalive eventually kills the socket, around two hours later. The user-visible symptom matches what's reported in #42: the ice adapter "doesn't connect to other players" because the FAF client's adapter-ready orchestration trips its own timeouts long before startup returns.Why this is safe
Telemetry is a debug-only observability channel:
ice-adapter/src/main/java/com/faforever/iceadapter/telemetry/implementOutgoingMessageV1— strictly one-way, adapter → server.TelemetryDebugger.onMessage()only logs incoming messages; no code path consumes inbound telemetry data.Debug.remove(this)runs; subsequentdebug().peerStateChanged(...)etc. iterate an empty debugger list and are no-ops.GameSessionorPeerIceModulewaits on or queries telemetry state.Fix
Run the connect on a virtual thread so
startupComplete()returns immediately. The pre-existing reconnect loop insendingLoop()already handles transient drops; queued telemetry messages catch up once the socket opens becausemessageQueueis unbounded.Two small additional cleanups in the same diff:
catch (InterruptedException e)block was missing areturn, so an interrupted startup fell through tosendMessage(new RegisterAsPeer(...)), queueing telemetry on a never-opening socket.Thread.currentThread().interrupt()per Java best practice.Verification
Verified locally on a JDK 21 / Gradle 8.8 build:
@Timeout(10s)kills it.:ice-adapter:check(compile + spotlessCheck) is green.The JUnit test isn't part of this PR —
ice-adapterdoesn't have a JUnit setup yet, and adding one alongside a one-file fix felt like scope creep. Happy to send a follow-up PR introducing JUnit + the regression test if maintainers want it.Fixes #42
Co-authored with Claude (Anthropic); reviewed and locally verified by me.
Summary by CodeRabbit