Copilot commented on code in PR #15914:
URL: https://github.com/apache/dubbo/pull/15914#discussion_r2762209782
##########
dubbo-config/dubbo-config-spring/src/main/java/org/apache/dubbo/config/spring/context/DubboDeployApplicationListener.java:
##########
@@ -134,80 +163,130 @@ 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 boolean isAutoStartup() {
+ return true;
}
@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 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());
+ running.set(false);
+ } 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);
+ }
}
}
}
- private void onContextRefreshedEvent(ContextRefreshedEvent event) {
- ModuleDeployer deployer = moduleModel.getDeployer();
- Assert.notNull(deployer, "Module deployer is null");
- Object singletonMutex =
LockUtils.getSingletonMutex(applicationContext);
- // start module
- Future future = null;
- synchronized (singletonMutex) {
- future = deployer.start();
- }
+ @Override
+ public void stop() {
+ stopInternal();
+ }
- // if the module does not start in background, await finish
- if (!deployer.isBackground()) {
+ @Override
+ public void stop(@NonNull Runnable callback) {
+ try {
+ stopInternal();
+ } finally {
try {
- future.get();
- } catch (InterruptedException e) {
+ callback.run();
+ } catch (Throwable t) {
logger.warn(
- CONFIG_FAILED_START_MODEL,
- "",
- "",
- "Interrupted while waiting for dubbo module start: " +
e.getMessage());
- } catch (Exception e) {
- logger.warn(
- CONFIG_FAILED_START_MODEL,
- "",
- "",
- "An error occurred while waiting for dubbo module
start: " + e.getMessage(),
- e);
+ CONFIG_STOP_DUBBO_ERROR, "", "", "Exception while
executing SmartLifecycle stop callback", t);
}
}
}
Review Comment:
There is no test coverage for the `stop(Runnable callback)` method, which is
part of the SmartLifecycle interface. Consider adding a test that verifies: 1)
the callback is invoked even if stopInternal() throws an exception, 2) the
callback is invoked after stopInternal() completes, and 3) exceptions from the
callback are caught and logged without breaking the shutdown process. This
would ensure the SmartLifecycle contract is properly implemented.
##########
dubbo-config/dubbo-config-spring/src/test/java/org/apache/dubbo/config/spring/context/DubboShutdownPhaseTest.java:
##########
@@ -0,0 +1,96 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.dubbo.config.spring.context;
+
+import org.apache.dubbo.config.bootstrap.DubboBootstrap;
+import org.apache.dubbo.config.spring.SysProps;
+
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.springframework.context.support.ClassPathXmlApplicationContext;
+
+class DubboShutdownPhaseTest {
+
+ private static final String PHASE_KEY = "dubbo.spring.shutdown.phase";
+ private static final int DEFAULT_PHASE = Integer.MIN_VALUE + 2000;
+
+ @AfterEach
+ void tearDown() {
+ DubboBootstrap.getInstance().stop();
+ SysProps.clear();
+ System.clearProperty(PHASE_KEY);
+ System.clearProperty("dubbo.protocol.name");
+ System.clearProperty("dubbo.registry.address");
+ }
+
+ @Test
+ void testDefaultPhase() {
+ try (ClassPathXmlApplicationContext context =
+ new
ClassPathXmlApplicationContext("org/apache/dubbo/config/spring/demo-provider.xml"))
{
+ context.start();
+ DubboDeployApplicationListener listener =
context.getBean(DubboDeployApplicationListener.class);
+ Assertions.assertEquals(DEFAULT_PHASE, listener.getPhase());
+ }
+ }
+
+ @Test
+ void testValidConfiguredPhase() {
+ System.setProperty(PHASE_KEY, "100");
+
+ try (ClassPathXmlApplicationContext context =
+ new
ClassPathXmlApplicationContext("org/apache/dubbo/config/spring/demo-provider.xml"))
{
+ context.start();
+ DubboDeployApplicationListener listener =
context.getBean(DubboDeployApplicationListener.class);
+ Assertions.assertEquals(100, listener.getPhase());
+ }
+ }
+
+ @Test
+ void testInvalidConfiguredPhase() {
+ System.setProperty(PHASE_KEY, "invalid-text");
+
+ try (ClassPathXmlApplicationContext context =
+ new
ClassPathXmlApplicationContext("org/apache/dubbo/config/spring/demo-provider.xml"))
{
+ context.start();
+ DubboDeployApplicationListener listener =
context.getBean(DubboDeployApplicationListener.class);
+ Assertions.assertEquals(
+ DEFAULT_PHASE,
+ listener.getPhase(),
+ "Default shutdown phase should be used when no
configuration is provided");
Review Comment:
The assertion message states "Default shutdown phase should be used when no
configuration is provided", but the test actually provides an invalid
configuration value ("invalid-text"). The message should be updated to:
"Default shutdown phase should be used when an invalid configuration value is
provided" to accurately reflect what the test is verifying.
```suggestion
"Default shutdown phase should be used when an invalid
configuration value is provided");
```
##########
dubbo-config/dubbo-config-spring/src/test/java/org/apache/dubbo/config/spring/context/DubboShutdownPhaseTest.java:
##########
@@ -0,0 +1,96 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.dubbo.config.spring.context;
+
+import org.apache.dubbo.config.bootstrap.DubboBootstrap;
+import org.apache.dubbo.config.spring.SysProps;
+
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.springframework.context.support.ClassPathXmlApplicationContext;
+
+class DubboShutdownPhaseTest {
+
+ private static final String PHASE_KEY = "dubbo.spring.shutdown.phase";
+ private static final int DEFAULT_PHASE = Integer.MIN_VALUE + 2000;
+
+ @AfterEach
+ void tearDown() {
+ DubboBootstrap.getInstance().stop();
+ SysProps.clear();
+ System.clearProperty(PHASE_KEY);
+ System.clearProperty("dubbo.protocol.name");
+ System.clearProperty("dubbo.registry.address");
+ }
+
+ @Test
+ void testDefaultPhase() {
+ try (ClassPathXmlApplicationContext context =
+ new
ClassPathXmlApplicationContext("org/apache/dubbo/config/spring/demo-provider.xml"))
{
+ context.start();
+ DubboDeployApplicationListener listener =
context.getBean(DubboDeployApplicationListener.class);
+ Assertions.assertEquals(DEFAULT_PHASE, listener.getPhase());
+ }
+ }
+
+ @Test
+ void testValidConfiguredPhase() {
+ System.setProperty(PHASE_KEY, "100");
+
+ try (ClassPathXmlApplicationContext context =
+ new
ClassPathXmlApplicationContext("org/apache/dubbo/config/spring/demo-provider.xml"))
{
+ context.start();
+ DubboDeployApplicationListener listener =
context.getBean(DubboDeployApplicationListener.class);
+ Assertions.assertEquals(100, listener.getPhase());
+ }
+ }
+
+ @Test
+ void testInvalidConfiguredPhase() {
+ System.setProperty(PHASE_KEY, "invalid-text");
+
+ try (ClassPathXmlApplicationContext context =
+ new
ClassPathXmlApplicationContext("org/apache/dubbo/config/spring/demo-provider.xml"))
{
+ context.start();
+ DubboDeployApplicationListener listener =
context.getBean(DubboDeployApplicationListener.class);
+ Assertions.assertEquals(
+ DEFAULT_PHASE,
+ listener.getPhase(),
+ "Default shutdown phase should be used when no
configuration is provided");
+ }
+ }
+
+ @Test
+ void testBoundaryValuePhase() {
+ System.setProperty(PHASE_KEY, String.valueOf(Integer.MIN_VALUE));
+
+ try (ClassPathXmlApplicationContext context =
+ new
ClassPathXmlApplicationContext("org/apache/dubbo/config/spring/demo-provider.xml"))
{
+ context.start();
+ DubboDeployApplicationListener listener =
context.getBean(DubboDeployApplicationListener.class);
+ Assertions.assertEquals(Integer.MIN_VALUE + 1,
listener.getPhase());
+ }
+ }
+
+ @BeforeEach
+ void setupDubboEnv() {
+ System.setProperty("dubbo.protocol.name", "dubbo");
+ System.setProperty("dubbo.registry.address", "N/A");
+ }
Review Comment:
The @BeforeEach method `setupDubboEnv()` is placed after all @Test methods
and @AfterEach. While this is functionally correct in JUnit 5, it's a
convention to place @BeforeEach methods before @Test methods for better
readability. Consider moving this method to appear before the first @Test
method (after line 41).
##########
dubbo-config/dubbo-config-spring/src/test/java/org/apache/dubbo/config/spring/context/DubboDeployApplicationListenerTest.java:
##########
@@ -0,0 +1,127 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.dubbo.config.spring.context;
+
+import org.apache.dubbo.common.deploy.ModuleDeployer;
+import org.apache.dubbo.config.bootstrap.DubboBootstrap;
+import org.apache.dubbo.config.spring.SysProps;
+import org.apache.dubbo.rpc.model.ModuleModel;
+
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Future;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.springframework.context.support.ClassPathXmlApplicationContext;
+import org.springframework.context.support.GenericApplicationContext;
+import org.springframework.test.util.ReflectionTestUtils;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+class DubboDeployApplicationListenerTest {
+
+ private static final String RESOURCE =
"org/apache/dubbo/config/spring/demo-provider.xml";
+
+ @AfterEach
+ void tearDown() {
+ DubboBootstrap.getInstance().stop();
+ SysProps.clear();
+ System.clearProperty("dubbo.protocol.name");
+ System.clearProperty("dubbo.registry.address");
+ }
+
+ @Test
+ void testIntegrationLifecycle() {
+ try (ClassPathXmlApplicationContext context = new
ClassPathXmlApplicationContext(RESOURCE)) {
+ context.start();
+
+ DubboDeployApplicationListener listener =
context.getBean(DubboDeployApplicationListener.class);
+ Assertions.assertTrue(listener.isRunning(), "Dubbo Lifecycle
should be running after context start");
+
+ // Verify shutdown phase is configured low (to stop last)
+ Assertions.assertTrue(listener.getPhase() < 0, "Dubbo should use a
low phase to stop last");
+
+ context.close();
+ Assertions.assertFalse(listener.isRunning(), "Dubbo Lifecycle
should stop after context close");
+ }
+ }
+
+ @Test
+ void testStartMethodInterrupted() throws ExecutionException,
InterruptedException {
+ // Expectations: Interrupt status is preserved
+ DubboDeployApplicationListener listener = new
DubboDeployApplicationListener();
+ ModuleModel mockModuleModel = mock(ModuleModel.class);
+ ModuleDeployer mockDeployer = mock(ModuleDeployer.class);
+ Future mockFuture = mock(Future.class);
+
+ when(mockModuleModel.getDeployer()).thenReturn(mockDeployer);
+ when(mockDeployer.start()).thenReturn(mockFuture);
+ when(mockDeployer.isBackground()).thenReturn(false);
+ when(mockFuture.get()).thenThrow(new InterruptedException("Simulated
Interrupt"));
+
+ ReflectionTestUtils.setField(listener, "moduleModel", mockModuleModel);
+ GenericApplicationContext context = new GenericApplicationContext();
+ context.refresh();
+
+ ReflectionTestUtils.setField(listener, "applicationContext", context);
+ ReflectionTestUtils.setField(listener, "running", new
AtomicBoolean(false));
+
+ listener.start();
+
+ Assertions.assertFalse(listener.isRunning(), "Running state should
reset on interrupt");
+ Assertions.assertTrue(Thread.currentThread().isInterrupted(),
"Interrupt status should be preserved");
+ Thread.interrupted();
+ }
+
+ @Test
+ void testStartMethodException() throws ExecutionException,
InterruptedException {
+ // Expectations: Exception does not escape start()
+ DubboDeployApplicationListener listener = new
DubboDeployApplicationListener();
+ ModuleModel mockModuleModel = mock(ModuleModel.class);
+ ModuleDeployer mockDeployer = mock(ModuleDeployer.class);
+ Future mockFuture = mock(Future.class);
+
+ when(mockModuleModel.getDeployer()).thenReturn(mockDeployer);
+ when(mockDeployer.start()).thenReturn(mockFuture);
+ when(mockDeployer.isBackground()).thenReturn(false);
+ when(mockFuture.get()).thenThrow(new RuntimeException("Simulated
Failure"));
+
+ ReflectionTestUtils.setField(listener, "moduleModel", mockModuleModel);
+ GenericApplicationContext context = new GenericApplicationContext();
+ context.refresh();
+
+ ReflectionTestUtils.setField(listener, "applicationContext", context);
+ ReflectionTestUtils.setField(listener, "running", new
AtomicBoolean(false));
+
+ listener.start();
+
+ Assertions.assertFalse(listener.isRunning(), "Running state should
reset on exception");
+ verify(mockDeployer, times(1)).start();
+ }
+
+ @BeforeEach
+ void setupDubboEnv() {
+ System.setProperty("dubbo.protocol.name", "dubbo");
+ System.setProperty("dubbo.registry.address", "N/A");
+ }
Review Comment:
The @BeforeEach method `setupDubboEnv()` is placed after all @Test methods
and @AfterEach. While this is functionally correct in JUnit 5, it's a
convention to place @BeforeEach methods before @Test methods for better
readability. Consider moving this method to appear before the first @Test
method (after line 51).
--
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]
