-
Notifications
You must be signed in to change notification settings - Fork 26.6k
Refactor Dubbo lifecycle to use SmartLifecycle #15914
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 3.3
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this 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>withSmartLifecycleinterface implementation - Introduced configurable shutdown phase with a default low value (Integer.MIN_VALUE + 2000) to ensure Dubbo stops last
- Refactored event handling from
onApplicationEvent()tostart(),stop(), and lifecycle management methods
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...ing/src/main/java/org/apache/dubbo/config/spring/context/DubboDeployApplicationListener.java
Outdated
Show resolved
Hide resolved
...ing/src/main/java/org/apache/dubbo/config/spring/context/DubboDeployApplicationListener.java
Show resolved
Hide resolved
...ing/src/main/java/org/apache/dubbo/config/spring/context/DubboDeployApplicationListener.java
Show resolved
Hide resolved
...ing/src/main/java/org/apache/dubbo/config/spring/context/DubboDeployApplicationListener.java
Show resolved
Hide resolved
...ing/src/main/java/org/apache/dubbo/config/spring/context/DubboDeployApplicationListener.java
Outdated
Show resolved
Hide resolved
...ing/src/main/java/org/apache/dubbo/config/spring/context/DubboDeployApplicationListener.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
| 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); | ||
| } | ||
| } | ||
| } |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
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.
| Assertions.assertEquals( | ||
| DEFAULT_PHASE, | ||
| listener.getPhase(), | ||
| "Default shutdown phase should be used when no configuration is provided"); |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
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.
| "Default shutdown phase should be used when no configuration is provided"); | |
| "Default shutdown phase should be used when an invalid configuration value is provided"); |
| @BeforeEach | ||
| void setupDubboEnv() { | ||
| System.setProperty("dubbo.protocol.name", "dubbo"); | ||
| System.setProperty("dubbo.registry.address", "N/A"); | ||
| } |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
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).
| @BeforeEach | ||
| void setupDubboEnv() { | ||
| System.setProperty("dubbo.protocol.name", "dubbo"); | ||
| System.setProperty("dubbo.registry.address", "N/A"); | ||
| } |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
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).
zrlw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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