Skip to content
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

Closed
wants to merge 22 commits into from

Conversation

Chenjp
Copy link
Contributor

@Chenjp Chenjp commented Jul 15, 2024

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 to DubboBootstrap.

Update DubboShutdownHookTest to add more internal check and improve code coverage.

Verifying this change

Checklist

  • Make sure there is a GitHub_issue field for the change (usually before you start working on it). Trivial changes like typos do not require a GitHub issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Check if is necessary to patch to Dubbo 3 if you are work on Dubbo 2.7
  • Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add sample in dubbo samples project.
  • Add some description to dubbo-website project if you are requesting to add a feature.
  • GitHub Actions works fine on your own branch.
  • If this contribution is large, please follow the Software Donation Guide.

When we register DubboShutdownHook, actions-on-exit are expected. So we should not disable the Hook.
@codecov-commenter
Copy link

codecov-commenter commented Jul 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.12%. Comparing base (3e53e32) to head (9d32562).
Report is 82 commits behind head on 3.2.

Additional details and impacted files
@@            Coverage Diff             @@
##              3.2   #14432      +/-   ##
==========================================
+ Coverage   70.11%   70.12%   +0.01%     
==========================================
  Files        1607     1608       +1     
  Lines       70192    70206      +14     
  Branches    10116    10116              
==========================================
+ Hits        49213    49231      +18     
- Misses      16324    16327       +3     
+ Partials     4655     4648       -7     

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

Copy link
Member

@AlbumenJ AlbumenJ left a 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.

@AlbumenJ
Copy link
Member

Ignore this exception would be better

@Chenjp
Copy link
Contributor Author

Chenjp commented Jul 17, 2024

We should remove the shutdown hook when Dubbo Application has been shutdowned by API.

@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.
TODO:

  1. move ApplicationModel (and sub-models) cleanup functions back to ApplicationModel#onDestroy
  2. Fwk model level cleanup - to FrameworkModel#onDestroy
  3. Runtime hook on process exit, shutdown dubbo engine gracefully: DubboShutdownHook supposed to do.

@AlbumenJ
Copy link
Member

  • move ApplicationModel (and sub-models) cleanup functions back to ApplicationModel#onDestroy
  • Fwk model level cleanup - to FrameworkModel#onDestroy
  • Runtime hook on process exit, shutdown dubbo engine gracefully: DubboShutdownHook supposed to do.

Agree

For those external managed moduleModel, checkAndWaitForTimeout.
Finally cleanup all resources arbitrarily when process exit.
@Chenjp Chenjp requested a review from AlbumenJ July 31, 2024 02:38
restore moduleModel external managed check to keep consistent with previous release.
add more log.
FIX: testDestoryWithModuleManagedExternally - add log assertion to ensure.
@Chenjp
Copy link
Contributor Author

Chenjp commented Aug 1, 2024

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 ScopeModel#internalIdcomment section. And if we follow the relationship rule strictly, then XxxModel#destroyed.compareAndSet should have never been used.

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.
improve code coverage.
enhance log message check.
@Chenjp Chenjp changed the title Remove unregister of ShutdownHook. fix #14429 Change DubboShutdownHook to singleton and process level Aug 5, 2024
@Chenjp Chenjp closed this Aug 6, 2024
@Chenjp Chenjp reopened this Aug 6, 2024
@Chenjp
Copy link
Contributor Author

Chenjp commented Aug 6, 2024

  • move ApplicationModel (and sub-models) cleanup functions back to ApplicationModel#onDestroy
  • Fwk model level cleanup - to FrameworkModel#onDestroy
  • Runtime hook on process exit, shutdown dubbo engine gracefully: DubboShutdownHook supposed to do.

Agree

Done. @AlbumenJ PTAL

…boSpringInitializer

DubboShutdownHook: enable two entrance support:DubboBootstrap and DubboSpringInitializer
recall prev ops.
DubboSpringInitializer update
…wn hook

register hook at application deployer and enable ignore listen shutdown hook
Copy link

sonarcloud bot commented Aug 27, 2024

@Chenjp Chenjp requested a review from AlbumenJ August 29, 2024 00:49
@Chenjp
Copy link
Contributor Author

Chenjp commented Sep 27, 2024

no more news.

@Chenjp Chenjp closed this Sep 27, 2024
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] application shutdown report error:java.lang.IllegalStateException: Shutdown in progress
3 participants