Copilot commented on code in PR #15914:
URL: https://github.com/apache/dubbo/pull/15914#discussion_r2644644488
##########
dubbo-config/dubbo-config-spring/src/main/java/org/apache/dubbo/config/spring/context/DubboDeployApplicationListener.java:
##########
@@ -134,80 +150,129 @@ private void publishApplicationEvent(DeployState state) {
applicationContext.publishEvent(new
DubboApplicationStateEvent(applicationModel, state));
}
- private void publishApplicationEvent(DeployState state, Throwable cause) {
- applicationContext.publishEvent(new
DubboApplicationStateEvent(applicationModel, state, cause));
+ private void publishApplicationEvent(Throwable cause) {
+ applicationContext.publishEvent(new
DubboApplicationStateEvent(applicationModel, DeployState.FAILED, cause));
}
private void publishModuleEvent(DeployState state) {
applicationContext.publishEvent(new DubboModuleStateEvent(moduleModel,
state));
}
- private void publishModuleEvent(DeployState state, Throwable cause) {
- applicationContext.publishEvent(new DubboModuleStateEvent(moduleModel,
state, cause));
+ private void publishModuleEvent(Throwable cause) {
+ applicationContext.publishEvent(new DubboModuleStateEvent(moduleModel,
DeployState.FAILED, cause));
}
@Override
- public void onApplicationEvent(ApplicationContextEvent event) {
- if (nullSafeEquals(applicationContext, event.getSource())) {
- if (event instanceof ContextRefreshedEvent) {
- onContextRefreshedEvent((ContextRefreshedEvent) event);
- } else if (event instanceof ContextClosedEvent) {
- onContextClosedEvent((ContextClosedEvent) event);
+ public boolean isAutoStartup() {
+ return true;
+ }
+
+ @Override
+ public void start() {
+ // Atomic check to ensure start logic runs only once.
+ if (running.compareAndSet(false, true)) {
+ ModuleDeployer deployer = moduleModel.getDeployer();
+ Assert.notNull(deployer, "Module deployer is null");
+ Object singletonMutex =
LockUtils.getSingletonMutex(applicationContext);
+
+ Future<?> future;
+ synchronized (singletonMutex) {
+ // Start the Dubbo module via the deployer.
+ future = deployer.start();
+ }
+
+ // If not running in background, wait for the startup to finish.
+ if (!deployer.isBackground()) {
+ try {
+ future.get();
+ } catch (InterruptedException e) {
+ // Preserve interrupt status
+ Thread.currentThread().interrupt();
+ logger.warn(
+ CONFIG_FAILED_START_MODEL,
+ "",
+ "",
+ "Interrupted while waiting for dubbo module start:
" + e.getMessage());
Review Comment:
After catching InterruptedException and restoring the interrupt status on
line 190, the code continues execution without returning or throwing an
exception. This means that even though the thread is interrupted, the start()
method completes normally and the running flag remains true. Consider either
rethrowing the exception or ensuring that the running flag is reset to false so
that isRunning() accurately reflects that the component didn't fully start.
```suggestion
"Interrupted while waiting for dubbo module
start: " + e.getMessage());
// Startup was interrupted; mark as not running and
return
running.set(false);
return;
```
##########
dubbo-config/dubbo-config-spring/src/main/java/org/apache/dubbo/config/spring/context/DubboDeployApplicationListener.java:
##########
@@ -32,39 +32,55 @@
import org.apache.dubbo.rpc.model.ModuleModel;
import java.util.concurrent.Future;
+import java.util.concurrent.atomic.AtomicBoolean;
+import org.jspecify.annotations.NonNull;
import org.springframework.beans.BeansException;
import org.springframework.context.ApplicationContext;
import org.springframework.context.ApplicationContextAware;
-import org.springframework.context.ApplicationListener;
-import org.springframework.context.event.ApplicationContextEvent;
-import org.springframework.context.event.ContextClosedEvent;
-import org.springframework.context.event.ContextRefreshedEvent;
+import org.springframework.context.SmartLifecycle;
import org.springframework.core.Ordered;
import static
org.apache.dubbo.common.constants.LoggerCodeConstants.CONFIG_FAILED_START_MODEL;
import static
org.apache.dubbo.common.constants.LoggerCodeConstants.CONFIG_STOP_DUBBO_ERROR;
-import static org.springframework.util.ObjectUtils.nullSafeEquals;
/**
* An ApplicationListener to control Dubbo application.
*/
-public class DubboDeployApplicationListener
- implements ApplicationListener<ApplicationContextEvent>,
ApplicationContextAware, Ordered {
+public class DubboDeployApplicationListener implements SmartLifecycle,
ApplicationContextAware, Ordered {
private static final ErrorTypeAwareLogger logger =
LoggerFactory.getErrorTypeAwareLogger(DubboDeployApplicationListener.class);
+ private static final String DUBBO_SHUTDOWN_PHASE_KEY =
"dubbo.spring.shutdown.phase";
+
private ApplicationContext applicationContext;
private ApplicationModel applicationModel;
private ModuleModel moduleModel;
+ private final AtomicBoolean running = new AtomicBoolean(false);
+ private volatile int shutdownPhase = Integer.MIN_VALUE + 2000;
+
@Override
- public void setApplicationContext(ApplicationContext applicationContext)
throws BeansException {
+ public void setApplicationContext(@NonNull ApplicationContext
applicationContext) throws BeansException {
this.applicationContext = applicationContext;
this.applicationModel =
DubboBeanUtils.getApplicationModel(applicationContext);
this.moduleModel = DubboBeanUtils.getModuleModel(applicationContext);
+
+ try {
+ // Parse the user-configured shutdown phase.
+ // Spring stops SmartLifecycle beans in descending phase order.
+ // To ensure Dubbo shuts down LAST, we use a very LOW phase value
by default.
+ String configured = ConfigurationUtils.getProperty(moduleModel,
DUBBO_SHUTDOWN_PHASE_KEY);
+ if (configured != null) {
+ shutdownPhase = Math.max(Integer.MIN_VALUE + 1,
Integer.parseInt(configured.trim()));
+ }
+ } catch (Exception e) {
+ logger.warn(
+ CONFIG_FAILED_START_MODEL, "", "", "Invalid value for
property: " + DUBBO_SHUTDOWN_PHASE_KEY, e);
Review Comment:
The error message "Invalid value for property: dubbo.spring.shutdown.phase"
could be more helpful by including what the invalid value was and what the
expected format is (e.g., "an integer between Integer.MIN_VALUE + 1 and
Integer.MAX_VALUE"). This would make debugging configuration issues easier for
users.
```suggestion
String configured = null;
try {
// Parse the user-configured shutdown phase.
// Spring stops SmartLifecycle beans in descending phase order.
// To ensure Dubbo shuts down LAST, we use a very LOW phase
value by default.
configured = ConfigurationUtils.getProperty(moduleModel,
DUBBO_SHUTDOWN_PHASE_KEY);
if (configured != null) {
shutdownPhase = Math.max(Integer.MIN_VALUE + 1,
Integer.parseInt(configured.trim()));
}
} catch (Exception e) {
String configuredValue = configured == null ? "null" : "'" +
configured.trim() + "'";
logger.warn(
CONFIG_FAILED_START_MODEL,
"",
"",
"Invalid value for property: " + DUBBO_SHUTDOWN_PHASE_KEY
+ ", configured value: " + configuredValue
+ ". Expected an integer between " +
(Integer.MIN_VALUE + 1) + " and " + Integer.MAX_VALUE + ".",
e);
```
##########
dubbo-config/dubbo-config-spring/src/main/java/org/apache/dubbo/config/spring/context/DubboDeployApplicationListener.java:
##########
@@ -32,39 +32,55 @@
import org.apache.dubbo.rpc.model.ModuleModel;
import java.util.concurrent.Future;
+import java.util.concurrent.atomic.AtomicBoolean;
+import org.jspecify.annotations.NonNull;
import org.springframework.beans.BeansException;
import org.springframework.context.ApplicationContext;
import org.springframework.context.ApplicationContextAware;
-import org.springframework.context.ApplicationListener;
-import org.springframework.context.event.ApplicationContextEvent;
-import org.springframework.context.event.ContextClosedEvent;
-import org.springframework.context.event.ContextRefreshedEvent;
+import org.springframework.context.SmartLifecycle;
import org.springframework.core.Ordered;
import static
org.apache.dubbo.common.constants.LoggerCodeConstants.CONFIG_FAILED_START_MODEL;
import static
org.apache.dubbo.common.constants.LoggerCodeConstants.CONFIG_STOP_DUBBO_ERROR;
-import static org.springframework.util.ObjectUtils.nullSafeEquals;
/**
* An ApplicationListener to control Dubbo application.
Review Comment:
The class-level documentation still refers to the old implementation as "An
ApplicationListener to control Dubbo application" but the class no longer
implements ApplicationListener. Update the documentation to reflect that this
is now a SmartLifecycle implementation that manages Dubbo startup and shutdown
within the Spring lifecycle, ensuring Dubbo stops last by default.
```suggestion
* A {@link org.springframework.context.SmartLifecycle} implementation that
manages
* Dubbo startup and shutdown within the Spring application lifecycle,
ensuring
* that Dubbo is stopped last by default.
```
##########
dubbo-config/dubbo-config-spring/src/main/java/org/apache/dubbo/config/spring/context/DubboDeployApplicationListener.java:
##########
@@ -134,80 +150,129 @@ private void publishApplicationEvent(DeployState state) {
applicationContext.publishEvent(new
DubboApplicationStateEvent(applicationModel, state));
}
- private void publishApplicationEvent(DeployState state, Throwable cause) {
- applicationContext.publishEvent(new
DubboApplicationStateEvent(applicationModel, state, cause));
+ private void publishApplicationEvent(Throwable cause) {
+ applicationContext.publishEvent(new
DubboApplicationStateEvent(applicationModel, DeployState.FAILED, cause));
}
private void publishModuleEvent(DeployState state) {
applicationContext.publishEvent(new DubboModuleStateEvent(moduleModel,
state));
}
- private void publishModuleEvent(DeployState state, Throwable cause) {
- applicationContext.publishEvent(new DubboModuleStateEvent(moduleModel,
state, cause));
+ private void publishModuleEvent(Throwable cause) {
+ applicationContext.publishEvent(new DubboModuleStateEvent(moduleModel,
DeployState.FAILED, cause));
}
@Override
- public void onApplicationEvent(ApplicationContextEvent event) {
- if (nullSafeEquals(applicationContext, event.getSource())) {
- if (event instanceof ContextRefreshedEvent) {
- onContextRefreshedEvent((ContextRefreshedEvent) event);
- } else if (event instanceof ContextClosedEvent) {
- onContextClosedEvent((ContextClosedEvent) event);
+ public boolean isAutoStartup() {
+ return true;
+ }
+
+ @Override
+ public void start() {
+ // Atomic check to ensure start logic runs only once.
+ if (running.compareAndSet(false, true)) {
+ ModuleDeployer deployer = moduleModel.getDeployer();
+ Assert.notNull(deployer, "Module deployer is null");
+ Object singletonMutex =
LockUtils.getSingletonMutex(applicationContext);
+
+ Future<?> future;
+ synchronized (singletonMutex) {
+ // Start the Dubbo module via the deployer.
+ future = deployer.start();
+ }
+
+ // If not running in background, wait for the startup to finish.
+ if (!deployer.isBackground()) {
+ try {
+ future.get();
+ } catch (InterruptedException e) {
+ // Preserve interrupt status
+ Thread.currentThread().interrupt();
+ logger.warn(
+ CONFIG_FAILED_START_MODEL,
+ "",
+ "",
+ "Interrupted while waiting for dubbo module start:
" + e.getMessage());
+ } catch (Exception e) {
+ logger.warn(CONFIG_FAILED_START_MODEL, "", "", "Error
starting dubbo module: " + e.getMessage(), e);
+ // If start fails, reset the running state to allow proper
shutdown
+ running.set(false);
+ }
}
}
}
Review Comment:
The SmartLifecycle implementation including the start(), stop(), and
isRunning() methods lacks test coverage. Since the repository has comprehensive
test coverage for Spring integration (as seen in multiple test files), consider
adding tests to verify: 1) start() is called automatically by Spring, 2) stop()
is called in the correct phase order, 3) the running state transitions
correctly, and 4) the behavior when start fails (e.g., interrupted or exception
scenarios).
##########
dubbo-config/dubbo-config-spring/src/main/java/org/apache/dubbo/config/spring/context/DubboDeployApplicationListener.java:
##########
@@ -32,39 +32,55 @@
import org.apache.dubbo.rpc.model.ModuleModel;
import java.util.concurrent.Future;
+import java.util.concurrent.atomic.AtomicBoolean;
+import org.jspecify.annotations.NonNull;
import org.springframework.beans.BeansException;
import org.springframework.context.ApplicationContext;
import org.springframework.context.ApplicationContextAware;
-import org.springframework.context.ApplicationListener;
-import org.springframework.context.event.ApplicationContextEvent;
-import org.springframework.context.event.ContextClosedEvent;
-import org.springframework.context.event.ContextRefreshedEvent;
+import org.springframework.context.SmartLifecycle;
import org.springframework.core.Ordered;
import static
org.apache.dubbo.common.constants.LoggerCodeConstants.CONFIG_FAILED_START_MODEL;
import static
org.apache.dubbo.common.constants.LoggerCodeConstants.CONFIG_STOP_DUBBO_ERROR;
-import static org.springframework.util.ObjectUtils.nullSafeEquals;
/**
* An ApplicationListener to control Dubbo application.
*/
-public class DubboDeployApplicationListener
- implements ApplicationListener<ApplicationContextEvent>,
ApplicationContextAware, Ordered {
+public class DubboDeployApplicationListener implements SmartLifecycle,
ApplicationContextAware, Ordered {
private static final ErrorTypeAwareLogger logger =
LoggerFactory.getErrorTypeAwareLogger(DubboDeployApplicationListener.class);
+ private static final String DUBBO_SHUTDOWN_PHASE_KEY =
"dubbo.spring.shutdown.phase";
+
private ApplicationContext applicationContext;
private ApplicationModel applicationModel;
private ModuleModel moduleModel;
+ private final AtomicBoolean running = new AtomicBoolean(false);
+ private volatile int shutdownPhase = Integer.MIN_VALUE + 2000;
+
@Override
- public void setApplicationContext(ApplicationContext applicationContext)
throws BeansException {
+ public void setApplicationContext(@NonNull ApplicationContext
applicationContext) throws BeansException {
this.applicationContext = applicationContext;
this.applicationModel =
DubboBeanUtils.getApplicationModel(applicationContext);
this.moduleModel = DubboBeanUtils.getModuleModel(applicationContext);
+
+ try {
+ // Parse the user-configured shutdown phase.
+ // Spring stops SmartLifecycle beans in descending phase order.
+ // To ensure Dubbo shuts down LAST, we use a very LOW phase value
by default.
+ String configured = ConfigurationUtils.getProperty(moduleModel,
DUBBO_SHUTDOWN_PHASE_KEY);
+ if (configured != null) {
+ shutdownPhase = Math.max(Integer.MIN_VALUE + 1,
Integer.parseInt(configured.trim()));
+ }
+ } catch (Exception e) {
+ logger.warn(
+ CONFIG_FAILED_START_MODEL, "", "", "Invalid value for
property: " + DUBBO_SHUTDOWN_PHASE_KEY, e);
+ }
Review Comment:
The new shutdown phase configuration feature (DUBBO_SHUTDOWN_PHASE_KEY) and
its parsing logic lacks test coverage. Since the repository has comprehensive
test coverage for other Spring integration features (as seen in
KeepRunningOnSpringClosedTest), consider adding tests to verify: 1) the default
phase value is used when no configuration is provided, 2) a valid configured
value is parsed correctly, and 3) invalid values are handled gracefully with
appropriate warnings.
##########
dubbo-config/dubbo-config-spring/src/main/java/org/apache/dubbo/config/spring/context/DubboDeployApplicationListener.java:
##########
@@ -32,39 +32,55 @@
import org.apache.dubbo.rpc.model.ModuleModel;
import java.util.concurrent.Future;
+import java.util.concurrent.atomic.AtomicBoolean;
+import org.jspecify.annotations.NonNull;
import org.springframework.beans.BeansException;
import org.springframework.context.ApplicationContext;
import org.springframework.context.ApplicationContextAware;
-import org.springframework.context.ApplicationListener;
-import org.springframework.context.event.ApplicationContextEvent;
-import org.springframework.context.event.ContextClosedEvent;
-import org.springframework.context.event.ContextRefreshedEvent;
+import org.springframework.context.SmartLifecycle;
import org.springframework.core.Ordered;
import static
org.apache.dubbo.common.constants.LoggerCodeConstants.CONFIG_FAILED_START_MODEL;
import static
org.apache.dubbo.common.constants.LoggerCodeConstants.CONFIG_STOP_DUBBO_ERROR;
-import static org.springframework.util.ObjectUtils.nullSafeEquals;
/**
* An ApplicationListener to control Dubbo application.
Review Comment:
The class name "DubboDeployApplicationListener" is misleading now that it no
longer implements ApplicationListener but instead implements SmartLifecycle.
While keeping the name may be intentional for backward compatibility or to
minimize changes, consider whether renaming to something like
"DubboLifecycleManager" or "DubboDeployLifecycle" would better reflect its
current purpose. If keeping the name for compatibility, consider adding a
comment explaining this decision.
```suggestion
* Controls the Dubbo application lifecycle within a Spring application
context.
* <p>
* This class currently integrates with Spring via {@link SmartLifecycle}
rather than
* {@code ApplicationListener}. The legacy name {@code
DubboDeployApplicationListener}
* is retained for backward compatibility (for example, existing
configuration or
* documentation that refers to this class by name).
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
