Aias00 commented on code in PR #7260:
URL: https://github.com/apache/shenyu/pull/7260#discussion_r4110269301
##########
shenyu-register-center/shenyu-register-client/shenyu-register-client-http/src/main/java/org/apache/shenyu/register/client/http/HttpClientRegisterRepository.java:
##########
@@ -238,11 +241,16 @@ private <T> void doHeartbeat(final T t, final String
path) {
RegisterUtils.doHeartBeat(GsonUtils.getInstance().toJson(t),
concat, Constants.HEARTBEAT, accessToken);
} catch (Exception e) {
LOGGER.error("HeartBeat admin url :{} is fail, will retry.",
server, e);
- if (i == serverList.size()) {
- throw new RuntimeException(e);
+ if (Objects.isNull(failure)) {
+ failure = new RuntimeException(e);
+ } else {
+ failure.addSuppressed(e);
}
}
}
+ if (Objects.nonNull(failure)) {
Review Comment:
Request changes - see review body. This exception has nowhere to go.
The heartbeat path has no retry queue and no exception boundary between here
and the scheduler:
```
ShenyuClientURIExecutorSubscriber.java:74 executor.scheduleAtFixedRate(()
-> uris.forEach(this::sendHeartbeat), 30, 10, SECONDS)
ShenyuClientURIExecutorSubscriber.java:126 private void sendHeartbeat(...)
{ repository.sendHeartbeat(uri); } // no try/catch
HttpClientRegisterRepository.java:138-143 public void
sendHeartbeat(URIRegisterDTO dto) { doHeartbeat(dto, URI_PATH); }
HttpClientRegisterRepository.java:251 throw failure; // NEW:
fires when ANY server failed
```
`ScheduledThreadPoolExecutor` suppresses all future runs of a periodic task
once it throws. Reproduced with two admins, one down, 50 ms period, 1 s window
(~20 expected ticks):
```
master rule (throw only when the LAST server failed): executed 21 ticks,
threw 0
this patch (throw when ANY server failed): executed 1 tick,
threw 1
```
One failover drill stops this client's heartbeats permanently, until the
process restarts; admin then ages the URI out and stops routing to it. It also
aborts `uris.forEach` mid-cycle, so every other registered URI loses that beat
too.
In fairness the hazard exists on master as well - when the *last* server
fails - but this patch promotes a rare edge case to the common path.
Two acceptable fixes: (a) do not throw from `doHeartbeat` at all, keeping
the accumulation only in `doRegister` where
`FailbackRegistryRepository.java:77-82` really does catch it and enqueue a
retry; or (b) keep throwing and wrap each URI's heartbeat in try/catch inside
the scheduled body, which also fixes the missing per-URI isolation.
--
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]
