-
Notifications
You must be signed in to change notification settings - Fork 26.4k
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
Change DubboShutdownHook to singleton and process level #14432
base: 3.2
Are you sure you want to change the base?
Conversation
When we register DubboShutdownHook, actions-on-exit are expected. So we should not disable the Hook.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 3.2 #14432 +/- ##
=======================================
Coverage 70.11% 70.11%
=======================================
Files 1607 1608 +1
Lines 70192 70210 +18
Branches 10116 10124 +8
=======================================
+ Hits 49213 49228 +15
- Misses 16324 16327 +3
Partials 4655 4655 ☔ View full report in Codecov by Sentry. |
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.
We should remove the shutdown hook when Dubbo Application has been shutdowned by API.
Ignore this exception would be better |
@AlbumenJ I am confused by DubboShutdownHook. Per constructor, dubboShutdownHook is ApplicationModel level. But following lines shows that its may impact other application models of same fwk model. // send readonly for shutdown hook
List<GracefulShutdown> gracefulShutdowns =
GracefulShutdown.getGracefulShutdowns(applicationModel.getFrameworkModel());
for (GracefulShutdown gracefulShutdown : gracefulShutdowns) {
gracefulShutdown.readonly();
} // if all FrameworkModels are destroyed, clean global static resources, shutdown dubbo completely According the above comment, dubbo server supports more than one FrameworkModel.
|
Agree |
For those external managed moduleModel, checkAndWaitForTimeout. Finally cleanup all resources arbitrarily when process exit.
add more log.
code smell
restore moduleModel external managed check to keep consistent with previous release. add more log.
FIX: testDestoryWithModuleManagedExternally - add log assertion to ensure.
After my commits, following stack traces shows that recursive destroy occur (framework->application->framework). @AlbumenJ may code refactoring is expected to match the productive-level design. We already define framework-application-module hierarchical relationship clearly in Request.setVersion(String) line: 95
HeaderExchangeServer.sendChannelReadOnlyEvent() line: 134
HeaderExchangeServer.close(int) line: 111
DubboProtocol.destroy() line: 610
ProtocolSecurityWrapper.destroy() line: 117
ProtocolListenerWrapper.destroy() line: 99
ProtocolFilterWrapper.destroy() line: 80
ProtocolSerializationWrapper.destroy() line: 60
InvokerCountWrapper.destroy() line: 55
FrameworkModelCleaner.destroyProtocols(FrameworkModel) line: 68
FrameworkModelCleaner.destroyFrameworkResources(FrameworkModel) line: 55
FrameworkModelCleaner.onDestroy(FrameworkModel) line: 47
FrameworkModelCleaner.onDestroy(ScopeModel) line: 1
FrameworkModel(ScopeModel).notifyProtocolDestroy() line: 155
FrameworkModel.tryDestroyProtocols() line: 280
ApplicationModel.onDestroy() line: 159
ApplicationModel(ScopeModel).destroy() line: 122
FrameworkModel.onDestroy() line: 129
FrameworkModel(ScopeModel).destroy() line: 122
FrameworkModel.destroyAll() line: 210
DubboShutdownHook.run() line: 128 |
reduce code complexity and enhance test case.
The hook's a part of Bootstrap.
Because DubboBootstrap does not follow it's original design - singleton, we have to introduce a key to prevent double-register.
code smell update.
improve code coverage. enhance log message check.
Done. @AlbumenJ PTAL |
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
package org.apache.dubbo.config.bootstrap; |
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.
Why move the package
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.
Currently, DubboShutdownHook
is instantiated and registered in DubboBootstrap
. That's why make DubboBootstrap
invisible to others except DubboBootstrap
. We can consider it as a part of Bootstrap.
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.
invisible
?
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.
In the same module and public method
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.
@AlbumenJ change DubboShutdownHook to a package type.
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.
@AlbumenJ Any new comments?
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.
In the same module and public method
@AlbumenJ If any other concern, will recall the move-package-ops.
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.
Still don't know why
…boSpringInitializer DubboShutdownHook: enable two entrance support:DubboBootstrap and DubboSpringInitializer
|
When we register DubboShutdownHook, actions-on-exit are expected. So we should not disable the Hook.
What is the purpose of the change
Change DubboShutdownHook to singleton, and make it as process level Hook. Fix #14429.
Brief changelog
As declared, ShutdownHook should perform process-exit ops, familiar with dubbo engine. Move
DubboShutdownHook#addShutdownHook
back toDubboBootstrap
.Update DubboShutdownHookTest to add more internal check and improve code coverage.
Verifying this change
Checklist