BobSong-dev commented on PR #7344:
URL: https://github.com/apache/shenyu/pull/7344#issuecomment-5882121322
> The core of this is right and I want it: silently swallowing a failed
config update is exactly what leaves a gateway running stale rules until
someone restarts it, and `close()` → health-check reconnect → `alreadySync =
false` → `MYSELF` is the correct way to force a full resynchronization. I
verified that chain in the head version: `onOpen:224-227` only sends `MYSELF`
when `alreadySync` is false, and `close():267` resets it, so the reconnect
really does re-pull everything.
>
> **Requesting changes on one thing: the terminal state at
`MAX_CONSECUTIVE_SYNC_FAILURES` has no recovery path.**
>
> `handleSyncFailure:404` calls `nowClose()`, and `nowClose():277-287` does
two things beyond closing the socket:
>
> ```java
> this.manuallyClosed.set(true);
> if (Objects.nonNull(timerTask)) { timerTask.cancel(); }
> ```
>
> `timerTask` is the 10-second wheel-timer task whose `doRun` is
`healthCheck()` (`:196-199`), and `healthCheck():291-297` is the **only** thing
that ever triggers a reconnect
(`RECONNECT_EXECUTOR.submit(this::doReconnect)`). `manuallyClosed` is set to
`true` here and never set back to `false` anywhere in the class.
>
> So after three consecutive failures this client is permanently dead: no
reconnect, no ping, no health check — and that includes the case the operator
actually wants, which is "the bad config was fixed on the admin side". Nothing
will ever pick the fix up; the only remedy is restarting the gateway. On master
the same situation logged a warning, dropped one message, and kept the
connection — so a later, good update still applied. I am not asking you to go
back to that, but the recovery loop has to survive the give-up.
>
> Concretely, one of these would work:
>
> * **Keep the timer alive.** At the cap, stop _closing on failure_ and go
back to log-and-ignore, but leave `healthCheck` running. The connection stays
up, and the next successful reconnect/MYSELF (or the next good message)
re-syncs and resets `consecutiveSyncFailures`. This bounds the reconnect storm
without giving up.
> * **Keep reconnecting, just stop re-syncing on failure.** Set a
`resyncDisabled` flag consulted by `handleSyncFailure`, let
`healthCheck`/`doReconnect` continue with its existing capped backoff
(`calculateBackoff`, up to 60s). A fixed admin config then recovers on its own.
> * If you genuinely want a terminal state, it needs to be observable rather
than a log line — but I would rather not have one at all here, because the
failure it detects is "config the gateway cannot apply", which is precisely the
state where you most want it to keep trying.
>
> Note that `testPoisonDataGivesUpAfterBoundedFailures` asserts `nowClose()`
is called, so whichever option you pick, that test changes with it.
>
> ### Two other things worth considering while you are in here
> 1. **Blast radius of the first failure.** The counter is reset by _any_
successful `executor` call (`:56`), which is good, but the very first failure
now drops the connection and forces a full re-pull of every config group. Under
a bad push, every gateway in the cluster does that at the same time. That is
the intended fix for #7316 so I am not blocking on it — just be sure the log at
`WARN` is enough for operators to correlate a reconnect storm with a bad
config, since it is now a normal-ish event rather than an exceptional one.
> 2. **The outer catch in `onMessage` now covers envelope parsing.**
`JsonUtils.jsonToMap(result)` and the `RUNNING_MODE` handling are inside the
same `try`, so a frame that is not parseable JSON — or any other shape the
client does not recognise — is now treated as a sync failure and closes the
connection, where master ignored it. If admin can emit any non-JSON frame
(heartbeat, error text), that is a new disconnect source.
Thanks for the careful review — the terminal state was indeed a dead end,
and option 1 is now
implemented (8e86395c5):
- At `MAX_CONSECUTIVE_SYNC_FAILURES` the client no longer calls
`nowClose()`. It keeps the
connection, stops closing on failure, and only logs further failures at
ERROR ("keeping the
connection and ignoring further failures until the next successful sync;
an admin-side fix
will be picked up by the running health check"). `healthCheck` and its
capped backoff stay
alive, and the next successful apply resets `consecutiveSyncFailures`,
restoring the normal
bounded recovery. No terminal state, no restart needed after an admin-side
fix.
- `testPoisonDataGivesUpAfterBoundedFailures` is rewritten as
`testPoisonDataKeepsConnectionAfterBoundedFailures`: failures 1-2 close
the connection,
failure 3+ is suppressed without closing, a successful apply resets the
counter, and a
following failure closes again (recovery restored). `nowClose` is verified
to never run.
- Your second point is also addressed: envelope parsing (`jsonToMap`),
`RUNNING_MODE`
handling, and frames that fail to parse into a recognized config message
are back to
log-and-ignore, so a non-JSON heartbeat or error frame is no longer a
disconnect source.
Only the failure to *apply* a recognized config message now triggers the
bounded resync.
- The first-failure WARN now names group, event type, consecutive-failure
count and the
server URI, and calls out a possible bad config push, so operators can
correlate a
reconnect burst with a bad push.
Verification: `./mvnw test checkstyle:check -pl
shenyu-sync-data-center/shenyu-sync-data-websocket -am`
— BUILD SUCCESS, ShenyuWebsocketClientTest 26/26 green. Not run locally: e2e.
--
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]
