Copilot commented on code in PR #16345: URL: https://github.com/apache/dubbo/pull/16345#discussion_r3472029336
########## dubbo-remoting/dubbo-remoting-netty4/src/test/java/org/apache/dubbo/remoting/transport/netty4/TripleGoAwayTest.java: ########## @@ -0,0 +1,98 @@ +/* + * 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.remoting.transport.netty4; + +import org.apache.dubbo.common.URL; +import org.apache.dubbo.common.constants.CommonConstants; +import org.apache.dubbo.common.utils.NetUtils; +import org.apache.dubbo.config.ApplicationConfig; +import org.apache.dubbo.config.context.ConfigManager; +import org.apache.dubbo.remoting.api.connection.AbstractConnectionClient; +import org.apache.dubbo.remoting.api.connection.ConnectionManager; +import org.apache.dubbo.remoting.api.connection.MultiplexProtocolConnectionManager; +import org.apache.dubbo.remoting.api.pu.DefaultPuHandler; +import org.apache.dubbo.rpc.model.ApplicationModel; +import org.apache.dubbo.rpc.model.ModuleModel; + +import java.time.Duration; + +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +import static org.apache.dubbo.common.constants.CommonConstants.EXECUTOR_MANAGEMENT_MODE_DEFAULT; +import static org.awaitility.Awaitility.await; + +public class TripleGoAwayTest { + + private static URL url; + + private static NettyPortUnificationServer server; + + private static ConnectionManager connectionManager; Review Comment: To make the test use an isolated model end-to-end (and later tear it down), keep a static FrameworkModel reference rather than relying on the global default model. ########## dubbo-remoting/dubbo-remoting-netty4/src/test/java/org/apache/dubbo/remoting/transport/netty4/TripleGoAwayTest.java: ########## @@ -0,0 +1,98 @@ +/* + * 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.remoting.transport.netty4; + +import org.apache.dubbo.common.URL; +import org.apache.dubbo.common.constants.CommonConstants; +import org.apache.dubbo.common.utils.NetUtils; +import org.apache.dubbo.config.ApplicationConfig; +import org.apache.dubbo.config.context.ConfigManager; +import org.apache.dubbo.remoting.api.connection.AbstractConnectionClient; +import org.apache.dubbo.remoting.api.connection.ConnectionManager; +import org.apache.dubbo.remoting.api.connection.MultiplexProtocolConnectionManager; +import org.apache.dubbo.remoting.api.pu.DefaultPuHandler; +import org.apache.dubbo.rpc.model.ApplicationModel; +import org.apache.dubbo.rpc.model.ModuleModel; + Review Comment: This test initializes its scope with ApplicationModel.defaultModel(), which is explicitly discouraged because it shares global process state and can become “broken” during teardown, causing unpredictable cross-test interference. Prefer creating an isolated FrameworkModel + frameworkModel.newApplication(), and destroy the FrameworkModel in `@AfterAll` to clean up shared resources. ########## dubbo-rpc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/transport/TripleGoAwayHandler.java: ########## @@ -35,12 +42,20 @@ public TripleGoAwayHandler() {} @Override public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception { if (msg instanceof Http2GoAwayFrame) { + Http2GoAwayFrame goAwayFrame = (Http2GoAwayFrame) msg; final ConnectionHandler connectionHandler = Review Comment: This handler logs GOAWAY errorCode/lastStreamId but always triggers the same “graceful migration” path for every GOAWAY. For non-advisory GOAWAYs (e.g., errorCode != 0 or lastStreamId != MAX_INT), continuing to route new streams on the old channel until migration completes can increase request failures; consider gating the graceful-migration behavior on these fields and falling back to the original close/clear + reconnect behavior otherwise. ########## dubbo-remoting/dubbo-remoting-netty4/src/test/java/org/apache/dubbo/remoting/transport/netty4/TripleGoAwayTest.java: ########## @@ -0,0 +1,98 @@ +/* + * 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.remoting.transport.netty4; + +import org.apache.dubbo.common.URL; +import org.apache.dubbo.common.constants.CommonConstants; +import org.apache.dubbo.common.utils.NetUtils; +import org.apache.dubbo.config.ApplicationConfig; +import org.apache.dubbo.config.context.ConfigManager; +import org.apache.dubbo.remoting.api.connection.AbstractConnectionClient; +import org.apache.dubbo.remoting.api.connection.ConnectionManager; +import org.apache.dubbo.remoting.api.connection.MultiplexProtocolConnectionManager; +import org.apache.dubbo.remoting.api.pu.DefaultPuHandler; +import org.apache.dubbo.rpc.model.ApplicationModel; +import org.apache.dubbo.rpc.model.ModuleModel; + +import java.time.Duration; + +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +import static org.apache.dubbo.common.constants.CommonConstants.EXECUTOR_MANAGEMENT_MODE_DEFAULT; +import static org.awaitility.Awaitility.await; + +public class TripleGoAwayTest { + + private static URL url; + + private static NettyPortUnificationServer server; + + private static ConnectionManager connectionManager; + + @BeforeAll + public static void init() throws Throwable { + int port = NetUtils.getAvailablePort(); + url = URL.valueOf("tri://127.0.0.1:" + port + "?foo=bar"); + ApplicationModel applicationModel = ApplicationModel.defaultModel(); + ApplicationConfig applicationConfig = new ApplicationConfig("provider-app"); + applicationConfig.setExecutorManagementMode(EXECUTOR_MANAGEMENT_MODE_DEFAULT); + applicationModel.getApplicationConfigManager().setApplication(applicationConfig); + ConfigManager configManager = new ConfigManager(applicationModel); + configManager.setApplication(applicationConfig); + configManager.getApplication(); + applicationModel.setConfigManager(configManager); + url = url.setScopeModel(applicationModel); + ModuleModel moduleModel = applicationModel.getDefaultModule(); + url = url.putAttribute(CommonConstants.SCOPE_MODEL, moduleModel); + server = new NettyPortUnificationServer(url, new DefaultPuHandler()); + server.bind(); + connectionManager = url.getOrDefaultFrameworkModel() + .getExtensionLoader(ConnectionManager.class) + .getExtension(MultiplexProtocolConnectionManager.NAME); + } + + @AfterAll + public static void close() { + try { + server.close(); + } catch (Throwable e) { + // ignored + } + } Review Comment: The `@AfterAll` cleanup closes the server but never destroys the FrameworkModel created for the test. Destroying it prevents shared executor/model resources from leaking across the test suite. ########## dubbo-remoting/dubbo-remoting-netty4/src/main/java/org/apache/dubbo/remoting/transport/netty4/AbstractNettyConnectionClient.java: ########## @@ -229,10 +230,16 @@ public void onConnected(Object channel) { connectedPromise.trySuccess(null); if (logger.isDebugEnabled()) { - logger.debug("Connection:{} connected", this); + logger.debug("Connection:{} connected, graceful migration complete", this); } Review Comment: onConnected() is invoked for the initial connect as well as for migrations; the new debug log message always says “graceful migration complete”, which is misleading when this is just the first connection establishment. Consider making the message conditional (only mention migration when there was a previous channel). ########## dubbo-remoting/dubbo-remoting-netty4/src/test/java/org/apache/dubbo/remoting/transport/netty4/TripleGoAwayTest.java: ########## @@ -0,0 +1,98 @@ +/* + * 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.remoting.transport.netty4; + +import org.apache.dubbo.common.URL; +import org.apache.dubbo.common.constants.CommonConstants; +import org.apache.dubbo.common.utils.NetUtils; +import org.apache.dubbo.config.ApplicationConfig; +import org.apache.dubbo.config.context.ConfigManager; +import org.apache.dubbo.remoting.api.connection.AbstractConnectionClient; +import org.apache.dubbo.remoting.api.connection.ConnectionManager; +import org.apache.dubbo.remoting.api.connection.MultiplexProtocolConnectionManager; +import org.apache.dubbo.remoting.api.pu.DefaultPuHandler; +import org.apache.dubbo.rpc.model.ApplicationModel; +import org.apache.dubbo.rpc.model.ModuleModel; + +import java.time.Duration; + +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +import static org.apache.dubbo.common.constants.CommonConstants.EXECUTOR_MANAGEMENT_MODE_DEFAULT; +import static org.awaitility.Awaitility.await; + +public class TripleGoAwayTest { + + private static URL url; + + private static NettyPortUnificationServer server; + + private static ConnectionManager connectionManager; + + @BeforeAll + public static void init() throws Throwable { + int port = NetUtils.getAvailablePort(); + url = URL.valueOf("tri://127.0.0.1:" + port + "?foo=bar"); + ApplicationModel applicationModel = ApplicationModel.defaultModel(); + ApplicationConfig applicationConfig = new ApplicationConfig("provider-app"); Review Comment: Avoid using ApplicationModel.defaultModel() in tests; create a fresh FrameworkModel and ApplicationModel for isolation. This also makes it possible to destroy the FrameworkModel in `@AfterAll` and prevent global state leakage into other tests. -- 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]
