Skip to content

fix(register): propagate unregister failures - #7144

Open
dengliming wants to merge 2 commits into
apache:masterfrom
dengliming:fix-6779-propagate-unregister-failure
Open

dengliming wants to merge 2 commits into
apache:masterfrom
dengliming:fix-6779-propagate-unregister-failure

Conversation

@dengliming

Copy link
Copy Markdown
Member

Summary

  • propagate the final admin-server failure from URI unregister operations
  • retain the original exception and stack trace in logs
  • add regression coverage for failed offline requests

Testing

  • mvn -q -pl shenyu-register-center/shenyu-register-client/shenyu-register-client-http -am -DskipTests=false -Dcheckstyle.skip=false -Dtest=HttpClientRegisterRepositoryTest -DfailIfNoTests=false test

Fixes #6779

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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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".

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (finally around executor.shutdown(), or split cleanup into its own hook) so propagation is safe, or
  • keep doUnregister non-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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] doUnregister swallows all offline exceptions — offline failures never retried, leaving stale upstreams

2 participants