wy471x commented on PR #6898: URL: https://github.com/apache/shenyu/pull/6898#issuecomment-5303577108
> ## Review: #6898 — fix: offload websocket reconnect from shared timer to dedicated executor > **Verdict: ✅ Approve (with a few non-blocking suggestions)** > > The core fix is correct and important, and the backoff/guard logic is well-tested. > > ### What's correct > * **Real root cause fixed.** `healthCheck()` runs on the shared wheel-timer's single-thread executor. The old code called `reconnectBlocking()` (a _blocking_ TCP connect) directly on that thread, so an unreachable admin would stall every other timer task (other clients' health checks, `masterCheck`) → missed heartbeats and delayed failover. Offloading to a dedicated `CachedThreadPool` removes the blockage. This is exactly the right fix for [[BUG] ShenyuWebsocketClient.healthCheck reconnectBlocking blocks the shared single-thread wheel timer #6844](https://github.com/apache/shenyu/issues/6844). > * **`AtomicBoolean reconnecting` guard prevents double-submit.** `compareAndSet(false, true)` in `healthCheck`, reset in `doReconnect`'s `finally`. While a reconnect is sleeping/in-flight, subsequent `healthCheck` calls are skipped. Correct. > * **Backoff math is sound.** `calculateBackoff()`: 0 failures → 0; else `MIN * (1 << min(failures-1, 10))` capped at 60s, plus up to +50% jitter. I verified the test bounds: failures=1 → [1000,1500]ms, =2 → [2000,3000], =4 → [8000,12000], =10 → capped 60000 + jitter ≤ 90000. All consistent with the assertions. > * **`InterruptedException` is respected** — `Thread.currentThread().interrupt()` restores status (tested), and the generic `catch (Exception)` increments backoff (capped at 10) and logs. `finally` always clears `reconnecting`. > * **Backoff reset on healthy path.** `healthCheck` sets `reconnectBackoff` to 0 when the socket is open, so a recovered connection doesn't carry stale backoff. > * **Good test coverage** — 11 new tests covering backoff zero/growth/cap/jitter, no-double-submit, reset-on-open, backoff increment/cap/reset-on-failure, backoff sleep, and interrupt preservation. > > ### Suggestions (non-blocking) > 1. **`RECONNECT_EXECUTOR` is a static, unbounded `newCachedThreadPool`.** For a gateway with several admin endpoints, a sustained reconnect storm could grow the thread count (reclaimed only after 60s idle). Consider either a _bounded_ pool or reusing ShenYu's existing managed executor (`ShenyuThreadFactory` + a fixed/limited pool) so reconnect threads are accounted for like the rest of the system. > 2. **Backoff is a floor, not strictly enforced during long connects.** `lastReconnectAttemptTime` is stamped _after_ `reconnectBlocking()` returns. If a connect itself takes longer than the computed `backoff` (e.g. multi-second TCP timeout), the next attempt's `waitMs` goes negative and reconnects immediately. Acceptable as a floor, just flagging the semantics. > 3. **`testDoReconnect*` rely on the real `reconnectBlocking()` throwing** against `ws://localhost:9090` (no server). This is fine in CI but is technically environment-sensitive (and the connect attempt adds a few seconds of latency to those tests). If you ever see flakiness, stub `reconnectBlocking()` instead of calling through to the parent. > > ### Verdict > Approving. The fix addresses a genuine liveness bug (timer-thread blockage), and the retry/backoff/guard implementation matches its tests. Address the executor-bounding point as a follow-up if you want tighter resource control. Thank you for the code review on this PR. Fix: 1. Bounded executor — Replaced the unbounded newCachedThreadPool with a repo-standard ShenyuThreadPoolExecutor (core 1, max 8, 60s keep-alive) using MemorySafeTaskQueue + ShenyuThreadFactory + AbortPolicy, so reconnect threads are accounted for like the rest of the system. 2. Backoff semantics — lastReconnectAttemptTime is now stamped in a finally after reconnectBlocking() completes, so a slow TCP connect no longer consumes the backoff window and the next attempt strictly waits the full backoff. 3. Test isolation — testDoReconnect* now stub reconnectBlocking() with doThrow(...) instead of making real socket connections to ws://localhost:9090. -- 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]
