Skip to content

Conversation

@fu-cheng1
Copy link
Contributor

This change removes the logic in FailsafeLogger that appends the local host information to log messages. The motivation is that FailsafeLogger is used by the WelcomeLogoApplicationListener to print logs very early during application startup—before Spring’s configuration files are fully loaded. As a result, NetUtils resolves and caches the local host information prematurely. This prevents the application from re-evaluating the preferred network interface later based on user-defined configuration. To avoid this issue, the host resolution in FailsafeLogger has been removed.

A new class, DubboNetInterfaceConfigApplicationListener, has been introduced to retrieve the preferred network interface configuration from Spring’s Environment and set it as a system property—since NetUtils reads this value from environment variables. This listener listens to the ApplicationContextInitializedEvent, which ensures that the Spring Environment (including values from configuration centers) is fully prepared when the listener executes.

…face config from Spring Environment

- Removed local host info logging from FailsafeLogger to prevent premature NetUtils host resolution. This issue occurs because WelcomeLogoApplicationListener logs messages before Spring configuration is fully loaded, causing NetUtils to cache incorrect host data and ignore preferred interface settings.
- Introduced DubboNetInterfaceConfigApplicationListener to extract `dubbo.network.interface.preferred` from the Spring Environment and inject it into system properties. This listener uses ApplicationContextInitializedEvent to ensure the configuration (including from config centers) is available before NetUtils is triggered.
@codecov-commenter
Copy link

codecov-commenter commented Aug 1, 2025

Codecov Report

❌ Patch coverage is 88.88889% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.09%. Comparing base (af2625c) to head (1d18ef3).
⚠️ Report is 58 commits behind head on 3.3.

Files with missing lines Patch % Lines
...nt/DubboNetInterfaceConfigApplicationListener.java 88.23% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##                3.3   #15604      +/-   ##
============================================
+ Coverage     61.06%   61.09%   +0.02%     
- Complexity    11693    11694       +1     
============================================
  Files          1909     1910       +1     
  Lines         86783    86800      +17     
  Branches      13094    13098       +4     
============================================
+ Hits          52994    53030      +36     
+ Misses        28373    28356      -17     
+ Partials       5416     5414       -2     
Flag Coverage Δ
integration-tests-java21 32.94% <0.00%> (-0.06%) ⬇️
integration-tests-java8 33.10% <0.00%> (+0.04%) ⬆️
samples-tests-java21 32.75% <66.66%> (+0.02%) ⬆️
samples-tests-java8 30.46% <66.66%> (+0.01%) ⬆️
unit-tests-java11 59.09% <88.88%> (+0.01%) ⬆️
unit-tests-java17 58.82% <88.88%> (+0.04%) ⬆️
unit-tests-java21 58.77% <88.88%> (+<0.01%) ⬆️
unit-tests-java8 59.09% <88.88%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@zrlw zrlw added type/enhancement Everything related with code enhancement or performance java Pull requests that update Java code labels Aug 1, 2025
private String appendContextMessage(String msg) {
return " [DUBBO] " + msg + ", dubbo version: " + Version.getVersion() + ", current host: "
+ NetUtils.getLocalHost();
return " [DUBBO] " + msg + ", dubbo version: " + Version.getVersion();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if apply this pr, the current host information will be logged at where? could you provide your log information about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, if the printing of this host information is not removed, it will cause premature caching of the network interface card (NIC) that should be used in WelcomeLogoApplicationListener. The execution timing of WelcomeLogoApplicationListener may occur before Spring loads the configuration files, resulting in the NIC configuration specified in the configuration files failing to take effect in NetUtils.

Therefore, if host information logging is mandatory:

  1. Could the execution of WelcomeLogoApplicationListener be deferred?
  2. Alternatively, would it be acceptable to omit logging the host information?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the host information should be logged for existed exception checking apps, you'd better log it at other location instead of omitting it directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the execution timing of WelcomeLogoApplicationListener and re-enabled the logging of local host information.

…the ApplicationEnvironmentPreparedEvent phase to the ApplicationContextInitializedEvent phase. It is now triggered after DubboNetInterfaceConfigApplicationListener to ensure proper network interface configuration is applied. Additionally, restored the host information appending behavior in FailsafeLogger.
@zrlw zrlw requested a review from Copilot August 3, 2025 03:18
Copy link
Contributor

Copilot AI left a 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 PR removes premature host resolution from FailsafeLogger and introduces proper network interface configuration support through Spring Environment. The change addresses an issue where network interface configuration was being evaluated too early in the application startup process, preventing user-defined configuration from taking effect.

  • Introduces DubboNetInterfaceConfigApplicationListener to handle network interface configuration from Spring Environment
  • Changes WelcomeLogoApplicationListener to use ApplicationContextInitializedEvent instead of ApplicationEnvironmentPreparedEvent
  • Registers the new listener in Spring's auto-configuration

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
DubboNetInterfaceConfigApplicationListener.java New listener that reads network interface configuration from Spring Environment and sets system properties
spring.factories Registers the new network interface configuration listener
WelcomeLogoApplicationListener.java Changes event type to ensure proper timing after Spring Environment is fully initialized
DubboNetInterfaceConfigApplicationListenerTest.java Test coverage for the new network interface configuration functionality
Comments suppressed due to low confidence (2)

dubbo-spring-boot-project/dubbo-spring-boot/src/test/java/org/apache/dubbo/spring/boot/context/event/DubboNetInterfaceConfigApplicationListenerTest.java:64

  • The test uses a custom listener instead of testing the actual DubboNetInterfaceConfigApplicationListener. This doesn't verify that the real implementation works correctly with the ApplicationContextInitializedEvent.
    static class NetworkInterfaceApplicationListener

dubbo-spring-boot-project/dubbo-spring-boot/src/test/java/org/apache/dubbo/spring/boot/context/event/DubboNetInterfaceConfigApplicationListenerTest.java:65

  • The test listener uses ApplicationEnvironmentPreparedEvent while the actual implementation uses ApplicationContextInitializedEvent. This mismatch means the test doesn't verify the correct event handling timing.
            implements ApplicationListener<ApplicationEnvironmentPreparedEvent> {

@zrlw zrlw requested review from EarthChen, oxsean and zrlw August 3, 2025 03:18
@zrlw zrlw requested a review from RainYuY August 6, 2025 02:41
Copy link
Member

@RainYuY RainYuY left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zrlw zrlw merged commit 9718408 into apache:3.3 Aug 6, 2025
32 checks passed
MoritzArena pushed a commit to MoritzArena/dubbo that referenced this pull request Sep 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

java Pull requests that update Java code type/enhancement Everything related with code enhancement or performance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants