fix(register): propagate unregister failures - #7144
dengliming wants to merge 2 commits into
Conversation
| 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); |
There was a problem hiding this 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:
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 afinallyin the subscriber (or a separate hook) so cleanup is exception-proof, or - keep
doUnregisternon-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".
Aias00
left a comment
There was a problem hiding this comment.
Summary
doUnregister now rethrows when the last admin server in serverList fails, matching the existing doRegister / doHeartbeat behaviour, and logs the exception object instead of only e.getMessage().
Requesting changes - the propagation lands in a shutdown hook
Consistency with doRegister/doHeartbeat is the right instinct, but offline() has a different blast radius than persistURI(): there is no failback path behind it. The only production caller is a JVM shutdown hook in ShenyuClientURIExecutorSubscriber (shenyu-client-core):
ShutdownHookManager.get().addShutdownHook(new Thread(() -> {
final URIRegisterDTO offlineDTO = new URIRegisterDTO();
BeanUtils.copyProperties(uriRegisterDTO, offlineDTO);
offlineDTO.setEventType(EventType.OFFLINE);
shenyuClientRegisterRepository.offline(offlineDTO); // now throws
// shutdown heartbeat executor
if (!executor.isTerminated()) {
executor.shutdown(); // skipped on throw
}
}), 2);ShutdownHookManager wraps each hook in try { hook.run(); } catch (Throwable ex) { LOG.error(...); }, so the exception is swallowed and logged - it does not surface anywhere that can react to it. What it does do is abort the remainder of that hook, so executor.shutdown() no longer runs and the heartbeat executor leaks. Before this change the failure was logged and cleanup still happened.
Net: the propagated exception is never actionable, and it costs us clean shutdown. Please pick one:
- make the subscriber resilient (
finallyaroundexecutor.shutdown(), or split cleanup into its own hook) so propagation is safe, or - keep
doUnregisternon-throwing and surface the failure some other way.
The new test offlineShouldThrowWhenEveryServerFails is good coverage for whatever you decide; it just needs to be paired with the caller fix.
Also worth fixing (inline)
i == serverList.size() encodes "the last server failed", not "every server failed". With serverList = [A, B], if A succeeds and B fails we throw even though the unregister partly succeeded. That is the same semantics doRegister/doHeartbeat already have, so this PR is consistent - but the sibling test is named doPersistUriShouldThrowWhenEveryServerFails and only uses a single-server config, so the mismatch is currently untested. Either track a failure count and throw only when all failed, or align the naming/expectation.
Minor: a bare RuntimeException is used. ShenyuException would be more idiomatic, though doRegister/doHeartbeat also use RuntimeException, so changing it here alone would be inconsistent - your call.
Summary
Testing
mvn -q -pl shenyu-register-center/shenyu-register-client/shenyu-register-client-http -am -DskipTests=false -Dcheckstyle.skip=false -Dtest=HttpClientRegisterRepositoryTest -DfailIfNoTests=false testFixes #6779