Hi team Spring Team,
To open this issue more professionally and cleanly, I used AI to help. Please accept my apologize.
While reviewing the codebase, I came across EurekaServerInitializerConfiguration and noticed a few areas that could benefit from modernization, thread-safety improvements, and consistency.
Before diving into the details, I noticed the inline comment inside the start() method:
// TODO: is this class even needed now?
First and foremost, I wanted to ask if this class is slated for removal or deprecation. If it is going to stay, I would love to submit a PR to clean it up.
Here are the specific points I've identified:
1. Thread Visibility Issue (volatile flag)
The running boolean flag is modified inside a manually created background thread (new Thread(() -> {...}).start()), but it is read by the container via the isRunning() method (likely on a different thread). Currently, it lacks the volatile keyword, which could lead to visibility issues in a concurrent environment.
2. Interface Segregation (Event Publishing)
The class currently autowires the entire ApplicationContext just to publish two events (EurekaRegistryAvailableEvent and EurekaServerStartedEvent). To adhere better to the interface segregation principle, it would be cleaner to inject ApplicationEventPublisher instead of the full context.
3. Dependency Injection Style
The class heavily relies on field injection (@Autowired on fields). Refactoring this to Constructor Injection would make the class immutable, easier to test, and align with current Spring best practices.
4. Inconsistent Injection Mechanisms (Aware Interfaces vs. DI)
The class implements ServletContextAware to obtain the ServletContext, while simultaneously using @Autowired for other framework components like ApplicationContext. In modern Spring, ServletContext can simply be injected via standard dependency injection (preferably Constructor Injection). Removing the Aware interface would standardize the injection mechanism and reduce framework coupling.
5. Raw Thread Creation
Instead of spinning up a raw new Thread(...), it might be safer to utilize a Spring-managed TaskExecutor or ThreadPoolTaskExecutor if asynchronous startup is still strictly required here.
6. Missing Documentation (Javadoc)
The class currently lacks class-level Javadoc explaining its primary role within the Eureka lifecycle, which makes it harder for new contributors to understand its context.
Proposal
If you agree that this class is still needed, I am more than happy to draft a PR to:
- Add
volatile to the running flag.
- (Optional, based on your feedback) Refactor all field injections and
ServletContextAware into a single Constructor Injection block.
- Replace
ApplicationContext with ApplicationEventPublisher.
- Add descriptive Javadoc to clarify the component's role and lifecycle.
- (Optional, based on your feedback) Refactor the raw thread creation.
I'm looking forward to contributing to this fix and would love to help out with similar tech-debt/refactoring tasks across the repository in the future.
Let me know your thoughts. Thank You
Hi team Spring Team,
To open this issue more professionally and cleanly, I used AI to help. Please accept my apologize.
While reviewing the codebase, I came across
EurekaServerInitializerConfigurationand noticed a few areas that could benefit from modernization, thread-safety improvements, and consistency.Before diving into the details, I noticed the inline comment inside the
start()method:// TODO: is this class even needed now?First and foremost, I wanted to ask if this class is slated for removal or deprecation. If it is going to stay, I would love to submit a PR to clean it up.
Here are the specific points I've identified:
1. Thread Visibility Issue (
volatileflag)The
runningboolean flag is modified inside a manually created background thread (new Thread(() -> {...}).start()), but it is read by the container via theisRunning()method (likely on a different thread). Currently, it lacks thevolatilekeyword, which could lead to visibility issues in a concurrent environment.2. Interface Segregation (Event Publishing)
The class currently autowires the entire
ApplicationContextjust to publish two events (EurekaRegistryAvailableEventandEurekaServerStartedEvent). To adhere better to the interface segregation principle, it would be cleaner to injectApplicationEventPublisherinstead of the full context.3. Dependency Injection Style
The class heavily relies on field injection (
@Autowiredon fields). Refactoring this to Constructor Injection would make the class immutable, easier to test, and align with current Spring best practices.4. Inconsistent Injection Mechanisms (Aware Interfaces vs. DI)
The class implements
ServletContextAwareto obtain theServletContext, while simultaneously using@Autowiredfor other framework components likeApplicationContext. In modern Spring,ServletContextcan simply be injected via standard dependency injection (preferably Constructor Injection). Removing theAwareinterface would standardize the injection mechanism and reduce framework coupling.5. Raw Thread Creation
Instead of spinning up a raw
new Thread(...), it might be safer to utilize a Spring-managedTaskExecutororThreadPoolTaskExecutorif asynchronous startup is still strictly required here.6. Missing Documentation (Javadoc)
The class currently lacks class-level Javadoc explaining its primary role within the Eureka lifecycle, which makes it harder for new contributors to understand its context.
Proposal
If you agree that this class is still needed, I am more than happy to draft a PR to:
volatileto therunningflag.ServletContextAwareinto a single Constructor Injection block.ApplicationContextwithApplicationEventPublisher.I'm looking forward to contributing to this fix and would love to help out with similar tech-debt/refactoring tasks across the repository in the future.
Let me know your thoughts. Thank You