Change DubboShutdownHook to singleton and process level #14432

Chenjp · 2024-07-15T06:48:00Z

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 · 2024-07-15T07:41:05Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.11%. Comparing base (3e53e32) to head (5a1ba46).
Report is 35 commits behind head on 3.2.

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.
📢 Have feedback on the report? Share it here.

AlbumenJ

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

AlbumenJ · 2024-07-16T09:51:38Z

Ignore this exception would be better

Chenjp · 2024-07-17T06:23:04Z

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:

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.

AlbumenJ · 2024-07-23T02:59:00Z

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.

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.

Chenjp · 2024-08-01T11:49:21Z

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.

code smell update.

improve code coverage. enhance log message check.

format.

Chenjp · 2024-08-06T00:47:35Z

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

AlbumenJ · 2024-08-07T07:02:22Z

...nfig/dubbo-config-api/src/main/java/org/apache/dubbo/config/bootstrap/DubboShutdownHook.java

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.dubbo.config.bootstrap;


Why move the package

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.

invisible?

In the same module and public method

@AlbumenJ change DubboShutdownHook to a package type.

@AlbumenJ Any new comments?

In the same module and public method

@AlbumenJ If any other concern, will recall the move-package-ops.

Still don't know why

…boSpringInitializer DubboShutdownHook: enable two entrance support:DubboBootstrap and DubboSpringInitializer

sonarcloud · 2024-08-20T14:27:02Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
87.9% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Remove unregister of ShutdownHook. fix apache#14429

5a1ba46

When we register DubboShutdownHook, actions-on-exit are expected. So we should not disable the Hook.

AlbumenJ requested changes Jul 16, 2024

View reviewed changes

AlbumenJ mentioned this pull request Jul 16, 2024

[Bug] application shutdown report error：java.lang.IllegalStateException: Shutdown in progress #14429

Open

4 tasks

Chenjp added 2 commits July 30, 2024 13:44

Change DubboShutdownHook to process level singleton

cb0f3e7

For those external managed moduleModel, checkAndWaitForTimeout. Finally cleanup all resources arbitrarily when process exit.

Update DubboShutdownHook.java

82578f3

add more log.

Chenjp requested a review from AlbumenJ July 31, 2024 02:38

Chenjp added 5 commits July 31, 2024 12:23

Sonar code smell enhancement

087a0cd

code smell

Update DubboShutdownHook.java

193b1fd

Merge branch 'apache:3.2' into 3.2-fix-14429-shutdownHook

6e00be3

Update DubboShutdownHook.java

6c218ac

restore moduleModel external managed check to keep consistent with previous release. add more log.

Update DubboShutdownHookTest.java

1c7e698

FIX: testDestoryWithModuleManagedExternally - add log assertion to ensure.

Chenjp added 6 commits August 2, 2024 11:28

code refactoring

13d9c7b

reduce code complexity and enhance test case.

Move DubboShutdownHook to DubboBootstrap

bf8929b

The hook's a part of Bootstrap.

Update DubboShutdownHook.java

00cd98c

Because DubboBootstrap does not follow it's original design - singleton, we have to introduce a key to prevent double-register.

Update DubboShutdownHookTest.java

4042f93

code smell update.

add more tests

fddbae8

improve code coverage. enhance log message check.

Update DubboShutdownHookTest.java

4d9aac1

format.

Chenjp changed the title ~~Remove unregister of ShutdownHook. fix #14429~~ Change DubboShutdownHook to singleton and process level Aug 5, 2024

Chenjp closed this Aug 6, 2024

Chenjp reopened this Aug 6, 2024

AlbumenJ requested changes Aug 7, 2024

View reviewed changes

DubboShutdownHook: enable two entrance support:DubboBootstrap and Dub…

d3b8cff

…boSpringInitializer DubboShutdownHook: enable two entrance support:DubboBootstrap and DubboSpringInitializer

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change DubboShutdownHook to singleton and process level #14432

Change DubboShutdownHook to singleton and process level #14432

Chenjp commented Jul 15, 2024 •

edited

Loading

codecov-commenter commented Jul 15, 2024

AlbumenJ left a comment

AlbumenJ commented Jul 16, 2024

Chenjp commented Jul 17, 2024 •

edited

Loading

AlbumenJ commented Jul 23, 2024

Chenjp commented Aug 1, 2024 •

edited

Loading

Chenjp commented Aug 6, 2024

AlbumenJ Aug 7, 2024

Chenjp Aug 8, 2024

AlbumenJ Aug 9, 2024

AlbumenJ Aug 9, 2024

Chenjp Aug 15, 2024

Chenjp Aug 16, 2024

Chenjp Aug 19, 2024

AlbumenJ Aug 21, 2024

sonarcloud bot commented Aug 20, 2024

Change DubboShutdownHook to singleton and process level #14432

Are you sure you want to change the base?

Change DubboShutdownHook to singleton and process level #14432

Conversation

Chenjp commented Jul 15, 2024 • edited Loading

What is the purpose of the change

Brief changelog

Verifying this change

Checklist

codecov-commenter commented Jul 15, 2024

Codecov Report

AlbumenJ left a comment

Choose a reason for hiding this comment

AlbumenJ commented Jul 16, 2024

Chenjp commented Jul 17, 2024 • edited Loading

AlbumenJ commented Jul 23, 2024

Chenjp commented Aug 1, 2024 • edited Loading

Chenjp commented Aug 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Aug 20, 2024

Quality Gate passed

Chenjp commented Jul 15, 2024 •

edited

Loading

Chenjp commented Jul 17, 2024 •

edited

Loading

Chenjp commented Aug 1, 2024 •

edited

Loading