-
Notifications
You must be signed in to change notification settings - Fork 26.5k
Feature/triple http3 cleanup fix #15347
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
Feature/triple http3 cleanup fix #15347
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 3.3 #15347 +/- ##
============================================
+ Coverage 60.76% 60.77% +0.01%
+ Complexity 10916 10915 -1
============================================
Files 1886 1886
Lines 86127 86145 +18
Branches 12903 12906 +3
============================================
+ Hits 52335 52357 +22
+ Misses 28335 28329 -6
- Partials 5457 5459 +2
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:
|
.getBean(DefaultRequestMappingRegistry.class); | ||
if (registry != null) { | ||
registry.destroy(); | ||
System.out.println("DefaultRequestMappingRegistry destroyed after test"); |
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.
System.out in ut is useless and please use logger instead
} | ||
} catch (Exception e) { | ||
System.err.println("Cleanup error: " + e.getMessage()); | ||
} |
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.
Exception should be thrown instead of just logging the error.
…eptions in TripleHttp3ProtocolTest
@ankitshokeen I think the root cause is that DefaultRequestMappingRegistry as a bean, is not properly destroyed. It needs to implement org.apache.dubbo.common.resource.Disposable so that it can be destroyed together when the ApplicationModel is destroyed. |
@oxsean Please let me know if there's anything else you'd like adjusted! |
|
||
class TripleHttp3ProtocolTest { | ||
|
||
private ApplicationModel applicationModel; |
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 hold applicationModel here? can we get it by ApplicationModel.defaultModel()
?
@Test | ||
void testDemoProtocol() throws Exception { | ||
// Full Environment Reset - Optimized |
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.
Since AfterEach already destroys the applicationModel, why do we still need to destroy it here?
What is the purpose of the change?
This pull request resolves the duplicate service mapping warnings observed during the execution of
TripleHttp3ProtocolTest
.It ensures that each test is executed in a fully isolated environment by resetting both the
ApplicationModel
andFrameworkModel
before initializing test logic.By clearing previously registered services and protocol instances, this change guarantees cleaner test runs without interference or unexpected warnings.
Checklist