Copilot commented on code in PR #16285:
URL: https://github.com/apache/dubbo/pull/16285#discussion_r3271673906
##########
dubbo-remoting/dubbo-remoting-netty4/src/main/java/org/apache/dubbo/remoting/transport/netty4/NettyConnectionClient.java:
##########
@@ -124,7 +135,16 @@ protected void initChannel(SocketChannel ch) {
// set null but do not close this client, it will be
reconnecting in the future
ch.closeFuture().addListener(channelFuture ->
clearNettyChannel());
- // TODO support Socks5
+ // support Socks5
+ String socksProxyHost =
+
ConfigurationUtils.getProperty(getUrl().getOrDefaultApplicationModel(),
SOCKS_PROXY_HOST);
+ if (socksProxyHost != null &&
!isFilteredAddress(getUrl().getHost())) {
+ int socksProxyPort =
Integer.parseInt(ConfigurationUtils.getProperty(
+ getUrl().getOrDefaultApplicationModel(),
SOCKS_PROXY_PORT, DEFAULT_SOCKS_PROXY_PORT));
Review Comment:
The SOCKS proxy config parsing can throw at runtime: `socksProxyHost` is
only checked for `null` (but ConfigurationUtils trims, so an empty property
becomes ""), and `Integer.parseInt(...)` will throw `NumberFormatException` if
`socksProxyPort` is not a valid integer. That would fail channel initialization
and prevent connecting even when proxy settings are merely misconfigured.
Consider using `StringUtils.isNotBlank(socksProxyHost)` and parsing the port
defensively (try/catch + range validation with a fallback/default, optionally
logging a warning).
##########
dubbo-remoting/dubbo-remoting-netty4/src/main/java/org/apache/dubbo/remoting/transport/netty4/NettyConnectionClient.java:
##########
@@ -124,7 +135,16 @@ protected void initChannel(SocketChannel ch) {
// set null but do not close this client, it will be
reconnecting in the future
ch.closeFuture().addListener(channelFuture ->
clearNettyChannel());
- // TODO support Socks5
+ // support Socks5
+ String socksProxyHost =
+
ConfigurationUtils.getProperty(getUrl().getOrDefaultApplicationModel(),
SOCKS_PROXY_HOST);
+ if (socksProxyHost != null &&
!isFilteredAddress(getUrl().getHost())) {
+ int socksProxyPort =
Integer.parseInt(ConfigurationUtils.getProperty(
+ getUrl().getOrDefaultApplicationModel(),
SOCKS_PROXY_PORT, DEFAULT_SOCKS_PROXY_PORT));
+ Socks5ProxyHandler socks5ProxyHandler =
+ new Socks5ProxyHandler(new
InetSocketAddress(socksProxyHost, socksProxyPort));
Review Comment:
`new InetSocketAddress(socksProxyHost, socksProxyPort)` may perform a
blocking DNS resolution during `initChannel(...)` (Netty worker thread). To
avoid potential event-loop blocking, prefer
`InetSocketAddress.createUnresolved(...)` (and let Netty resolve during
connect) or otherwise ensure resolution happens off the event loop.
##########
dubbo-remoting/dubbo-remoting-netty4/src/main/java/org/apache/dubbo/remoting/transport/netty4/NettyConnectionClient.java:
##########
@@ -52,6 +56,12 @@
public final class NettyConnectionClient extends AbstractNettyConnectionClient
{
+ private static final String SOCKS_PROXY_HOST = "socksProxyHost";
+
+ private static final String SOCKS_PROXY_PORT = "socksProxyPort";
+
+ private static final String DEFAULT_SOCKS_PROXY_PORT = "1080";
Review Comment:
The SOCKS proxy constants and `isFilteredAddress(...)` logic duplicate the
existing implementation in `NettyClient`. Duplicating these keys/filters in
multiple clients risks divergence over time; consider extracting the proxy
configuration + filter logic into a shared utility (or a common constant
holder) used by both clients.
##########
dubbo-remoting/dubbo-remoting-netty4/src/main/java/org/apache/dubbo/remoting/transport/netty4/NettyConnectionClient.java:
##########
@@ -124,7 +135,16 @@ protected void initChannel(SocketChannel ch) {
// set null but do not close this client, it will be
reconnecting in the future
ch.closeFuture().addListener(channelFuture ->
clearNettyChannel());
- // TODO support Socks5
+ // support Socks5
+ String socksProxyHost =
+
ConfigurationUtils.getProperty(getUrl().getOrDefaultApplicationModel(),
SOCKS_PROXY_HOST);
+ if (socksProxyHost != null &&
!isFilteredAddress(getUrl().getHost())) {
+ int socksProxyPort =
Integer.parseInt(ConfigurationUtils.getProperty(
+ getUrl().getOrDefaultApplicationModel(),
SOCKS_PROXY_PORT, DEFAULT_SOCKS_PROXY_PORT));
+ Socks5ProxyHandler socks5ProxyHandler =
+ new Socks5ProxyHandler(new
InetSocketAddress(socksProxyHost, socksProxyPort));
+ pipeline.addFirst(socks5ProxyHandler);
+ }
Review Comment:
This new SOCKS5 support is not covered by existing netty4 transport tests
(no tests mention `Socks5ProxyHandler`). Please add a unit/integration test
that sets `socksProxyHost`/`socksProxyPort` and asserts the handler is inserted
(and ideally that a connection attempt targets the proxy first). This helps
prevent regressions for tri/HTTP2 connections.
--
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]
