Aias00 commented on code in PR #7144:
URL: https://github.com/apache/shenyu/pull/7144#discussion_r4060322972
##########
shenyu-register-center/shenyu-register-client/shenyu-register-client-http/src/main/java/org/apache/shenyu/register/client/http/HttpClientRegisterRepository.java:
##########
@@ -240,7 +242,10 @@ private <T> void doUnregister(final T t) {
RegisterUtils.doUnregister(GsonUtils.getInstance().toJson(t),
concat, accessToken);
// considering the situation of multiple clusters, we should
continue to execute here
} catch (Exception e) {
- LOGGER.error("Unregister admin url :{} is fail. cause:{}",
server, e.getMessage());
+ LOGGER.error("Unregister admin url :{} is fail.", server, e);
+ if (i == serverList.size()) {
+ throw new RuntimeException(e);
Review Comment:
This makes `doUnregister` consistent with `doRegister` / `doHeartbeat`,
which I agree with in principle. But the only production caller is a JVM
shutdown hook, and propagating here has a concrete side effect worth checking
before merge:
`ShenyuClientURIExecutorSubscriber` (shenyu-client-core) registers:
```java
ShutdownHookManager.get().addShutdownHook(new Thread(() -> {
...
shenyuClientRegisterRepository.offline(offlineDTO); // <-- now can
throw
// shutdown heartbeat executor
if (!executor.isTerminated()) {
executor.shutdown(); // <-- skipped if
the line above throws
}
}), 2);
```
`ShutdownHookManager` wraps each hook in `try { hook.run(); } catch
(Throwable ex) { LOG.error(...); }`, so the throw is swallowed and logged - but
it aborts the rest of *that* hook, so the heartbeat executor is never shut
down. Previously the unregister failure was logged and the cleanup still ran.
Net effect: the "propagation" never reaches anything that can act on it
(there is no failback for `offline`, unlike `persistURI`), and it costs us the
executor shutdown. Options:
- move the `executor.shutdown()` into a `finally` in the subscriber (or a
separate hook) so cleanup is exception-proof, or
- keep `doUnregister` non-throwing and just make the failure visible another
way.
If you go with propagation, please also cover the multi-server ordering
semantics noted in the review - `i == serverList.size()` means "the *last*
server failed", not "every server failed".
--
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]
