Skip to content

Refactor Dubbo lifecycle to use SmartLifecycle#15914

Open
somiljain2006 wants to merge 5 commits intoapache:3.3from
somiljain2006:shutdown_order_fix
Open

Refactor Dubbo lifecycle to use SmartLifecycle#15914
somiljain2006 wants to merge 5 commits intoapache:3.3from
somiljain2006:shutdown_order_fix

Conversation

@somiljain2006
Copy link

What is the purpose of the change?

To move Dubbo startup and shutdown management into SmartLifecycle and configure it to stop last. This change ensures Dubbo remains available until all other Spring-managed components have completed their shutdown, enabling a safe shutdown without runtime exceptions.

Fixes #15756

Checklist

  • Make sure there is a GitHub_issue field for the change.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit tests to verify your logic correction. If the new feature or significant change is committed, please remember to add a sample to the dubbo samples project.
  • Make sure GitHub actions can pass. Why the workflow is failing and how to fix it?

@codecov-commenter
Copy link

codecov-commenter commented Dec 23, 2025

Codecov Report

❌ Patch coverage is 69.69697% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.78%. Comparing base (f27d0d7) to head (3c918ba).

Files with missing lines Patch % Lines
...spring/context/DubboDeployApplicationListener.java 69.69% 16 Missing and 4 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##                3.3   #15914      +/-   ##
============================================
+ Coverage     60.75%   60.78%   +0.02%     
- Complexity    11751    11752       +1     
============================================
  Files          1951     1951              
  Lines         88992    89023      +31     
  Branches      13418    13418              
============================================
+ Hits          54070    54112      +42     
+ Misses        29362    29355       -7     
+ Partials       5560     5556       -4     
Flag Coverage Δ
integration-tests-java21 32.17% <45.45%> (-0.02%) ⬇️
integration-tests-java8 32.29% <45.45%> (-0.01%) ⬇️
samples-tests-java21 32.25% <45.45%> (+0.05%) ⬆️
samples-tests-java8 29.96% <45.45%> (+0.09%) ⬆️
unit-tests-java11 59.02% <69.69%> (+0.01%) ⬆️
unit-tests-java17 58.52% <69.69%> (+0.03%) ⬆️
unit-tests-java21 58.50% <69.69%> (-0.01%) ⬇️
unit-tests-java25 58.46% <69.69%> (-0.02%) ⬇️
unit-tests-java8 59.02% <69.69%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@zrlw zrlw requested a review from Copilot December 24, 2025 02:24
@zrlw zrlw added the type/enhancement Everything related with code enhancement or performance label Dec 24, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request refactors the Dubbo lifecycle management from an event-driven ApplicationListener pattern to Spring's SmartLifecycle interface. This ensures Dubbo remains available during application shutdown until all other Spring-managed components have completed their lifecycle, preventing runtime exceptions during graceful shutdown.

Key changes:

  • Replaced ApplicationListener<ApplicationContextEvent> with SmartLifecycle interface implementation
  • Introduced configurable shutdown phase with a default low value (Integer.MIN_VALUE + 2000) to ensure Dubbo stops last
  • Refactored event handling from onApplicationEvent() to start(), stop(), and lifecycle management methods

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +225 to 236
public void stop(@NonNull Runnable callback) {
try {
stopInternal();
} finally {
try {
future.get();
} catch (InterruptedException e) {
callback.run();
} catch (Throwable t) {
logger.warn(
CONFIG_FAILED_START_MODEL,
"",
"",
"Interrupted while waiting for dubbo module start: " + e.getMessage());
} catch (Exception e) {
logger.warn(
CONFIG_FAILED_START_MODEL,
"",
"",
"An error occurred while waiting for dubbo module start: " + e.getMessage(),
e);
CONFIG_STOP_DUBBO_ERROR, "", "", "Exception while executing SmartLifecycle stop callback", t);
}
}
}
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

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

There is no test coverage for the stop(Runnable callback) method, which is part of the SmartLifecycle interface. Consider adding a test that verifies: 1) the callback is invoked even if stopInternal() throws an exception, 2) the callback is invoked after stopInternal() completes, and 3) exceptions from the callback are caught and logged without breaking the shutdown process. This would ensure the SmartLifecycle contract is properly implemented.

Copilot uses AI. Check for mistakes.
Assertions.assertEquals(
DEFAULT_PHASE,
listener.getPhase(),
"Default shutdown phase should be used when no configuration is provided");
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

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

The assertion message states "Default shutdown phase should be used when no configuration is provided", but the test actually provides an invalid configuration value ("invalid-text"). The message should be updated to: "Default shutdown phase should be used when an invalid configuration value is provided" to accurately reflect what the test is verifying.

Suggested change
"Default shutdown phase should be used when no configuration is provided");
"Default shutdown phase should be used when an invalid configuration value is provided");

Copilot uses AI. Check for mistakes.
Comment on lines +91 to +95
@BeforeEach
void setupDubboEnv() {
System.setProperty("dubbo.protocol.name", "dubbo");
System.setProperty("dubbo.registry.address", "N/A");
}
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

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

The @beforeeach method setupDubboEnv() is placed after all @test methods and @AfterEach. While this is functionally correct in JUnit 5, it's a convention to place @beforeeach methods before @test methods for better readability. Consider moving this method to appear before the first @test method (after line 41).

Copilot uses AI. Check for mistakes.
Comment on lines +122 to +126
@BeforeEach
void setupDubboEnv() {
System.setProperty("dubbo.protocol.name", "dubbo");
System.setProperty("dubbo.registry.address", "N/A");
}
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

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

The @beforeeach method setupDubboEnv() is placed after all @test methods and @AfterEach. While this is functionally correct in JUnit 5, it's a convention to place @beforeeach methods before @test methods for better readability. Consider moving this method to appear before the first @test method (after line 51).

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@zrlw zrlw left a comment

Choose a reason for hiding this comment

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

LGTM

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

Labels

status/wait for another approve type/enhancement Everything related with code enhancement or performance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] DubboDeployApplicationListener shuts down Dubbo too early, causing the error: Directory of type RegistryDirectory already destroyed for service.

3 participants